[haiku-development] Re: ext3 Journal WIP patch
- From: Stephan Assmus <superstippi@xxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Fri, 16 Apr 2010 09:46:01 +0200
Hi Janito,
First of all, thanks for working on the patch and no worries about using
pastebin... :-)
On 2010-04-16 at 06:55:58 [+0200], Janito Ferreira Filho <jvffprog@xxxxxxxxxxx>
wrote:
> It probably contains errors, so if someone spots one, please tell me. As
> soon as I can get it to compile tomorrow, and if it's worthy to become a
> real patch, I'll post it on Trac. Feel free to post opinions, criticism,
> rants =), etc. Thanks in advance, and sorry for this "pastebin hack".
I cannot comment on what the code is supposed to do, but I can try to point out
coding style and some errors. Here is goes...
Line 14 - No blank line between copyright and header guard.
Line 21 - Two blank lines between sections (this rule is hard to get just
right, since sometimes opinion differ about what a section is... ;-)
Line 24, 32, 39 - commit_id -> commitID (no underscore in variable names)
Line 36 - elem -> element
Line 44 - spelling
All of HashRevokeManager.h -> Inconsistent asterix style. Most of us favor
"Type* name". The rule just says you need to be consistent.
Journal.cpp:
Line 102: One blank line between the header for this file (Journal.h) and the
rest of the headers.
Line 115: Why not init fRevokeManager in the constructor list? But better init
it in InitJournal(), use new(std::nothrow) and return B_NO_MEMORY in case
of allocation failure.
+Journal::InitJournal()
+{
+ status_t retValue;
+
+ if((retValue = _LoadSuperBlock()) != B_OK)
+ return retValue;
+
+ if((retValue = _Recover()) != B_OK)
+ return retValue;
+
+ return B_OK;
+}
This code here and also elsewhere in the file - too much C-style. You can
reword it like this:
Journal::InitJournal()
{
fRevokeManager = new(std::nothrow) HashRevokeManager;
if (fRevokeManager == NULL)
return B_NO_MEMORY;
status_t ret = _LoadSuperBlock();
if (ret != B_OK)
return ret;
ret = _Recover();
if (ret != B_OK)
return ret;
return B_OK;
}
Or like this:
Journal::InitJournal()
{
fRevokeManager = new(std::nothrow) HashRevokeManager;
if (fRevokeManager == NULL)
return B_NO_MEMORY;
status_t ret = _LoadSuperBlock();
if (ret == B_OK)
ret = _Recover();
return ret;
}
Note the space between "if" and "(".
Line 147-149 - strange use of parenthesis.
Method Journal::_LoadSuperBlock(), C-style code again. Declare variables where
you first need them.
Line 157 - superblock_pos -> superblockPos
Line 182 - missing semicolon
Line 193 - missing "return B_OK;"
I think if I read further, I could just point out the same coding style
improvements. You need to put a space between "if" and "(" and between "while"
and "(". Two blank lines between all methods. Variable names should not have
underscores, declare them where they are first needed.
Again, thanks a lot for working on the patch. I hope my suggestions already
help you to improve the code. The general flow of your code seems sound and
you are doing well with a lot of other coding style rules, but as I said I
cannot really comment on what the code does, and it would probably be
premature, since you need to get it into compilable state and perhaps find many
errors the compiler points out until you do.
Best regards,
-Stephan
Other related posts:
