[haiku-development] Re: StyledEdit
- From: "Ryan Leavengood" <leavengood@xxxxxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Mon, 3 Sep 2007 21:51:12 -0400
On 9/3/07, Axel Dörfler <axeld@xxxxxxxxxxxxxxxx> wrote:
>
> The patch is quite long - it's hard to review such patches. It would be
> nice if you could actually divide your patch in two separate ones: one
> that fixes the style issues, and the other that fixes the other
> problem.
Yes indeed smaller patches are easier for us to review and commit.
> Anyway, that's just a suggestion, I'm off to a vacation, anyway, I hope
> someone will step up to review it in the mean time.
I'll take care of it. A few comments though:
In ColorMenuItem.cpp there was an if like this:
if (NULL != menu) {
The coding guidelines specify not to use conditionals like this (where
the variable comes after what is being tested against.) In addition
since you are comparing against NULL you could just do:
if (menu) {
This is what I changed it to before committing.
Another small thing is I am not sure if you are using 4 space tabs
(our standard) because the adjustment of some tabs in a few files does
not line up right with 4 space tabs. In fact in seems you are using 8
space tabs. Please change your editor to use 4 space tabs. I've
corrected the files with the tabbing problems.
The guidelines also specify that there should not be any space between
the copyright text and header guard #ifdef in header files. You added
a newline here in a few cases, which I removed.
In a few files you moved a { on a switch to the next line. It should
have remained on the previous line.
Another { rule to keep in mind is it comes on the end of the line in
class declarations. You added a newline in a few cases. I've fixed
this too ;)
Now regarding the removing of the virtual keyword on several
overridden methods, I suppose this is fine since I doubt any of those
classes will ever be subclassed themselves. I'm not sure it has any
benefits though (I am no C++ expert though.)
I hope these small criticisms don't dampen your enthusiasm, but they
should be good to know since you are fixing style issues. Your
contribution is most welcome and I hope we can see more work from you!
:)
I will commit the code soon after some testing.
Regards,
Ryan
Other related posts:
