[haiku-development] Re: Set*UIColor Version 3b Patch
- From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Wed, 18 Nov 2015 21:18:36 +0100
Am 11.11.2015 um 23:48 schrieb looncraz:
I have cleaned up the commit history immensely, rebased off the
current master (after Julian Harnath's view layer commit).
I've reviewed the first part of the patches until line 2556 (right after
the documentation to B_FOLLOW_DEFAULT).
First, the still numerous coding style issues - the numbers are line
numbers of the patch file:
86, 104, 115, 132, 386, 469, 649, 681, 709, 744
coding style: single line if
297, 402
multiline for without curly braces
365, 454, 488, 577, 584
multiline if without curly braces
311, 403
extraneous parenthesis
167, 608, 1433, 1437, 1441
no space between comma and comment
817, 822, 1167, 1448-1458
wrong indentation
332, 872, 1478
missing blank line(s) after header
174 no tab, Z?
206 DMScheduledMessage is still a bad name
248 wrong indentation - operator is part of the name
301 why is count == 0 B_ERROR, and not B_BAD_VALUE?
346 missing space after //
422 "target" declaration should be within the for loop
742 extraneous space after ::
851 Using friend classes like this should be avoided; rather make
the
implementation/declaration private
931 return type gets its own line
997 please use either B_RELATIVE_TIMEOUT, or B_ABSOLUTE_TIMEOUT
1231 Protected methods don't get the underscore prefix, only private ones do
1240 We actually prefer to keep those to separate the methods from the
member fields.
1251 Patch 2/91 is missing? Instead of just writing "BColorMap" write
"BColorMap class added". There is no
reason to be that brief in there; you have 64 characters for a
summary
of your changes.
1367 Why is there no underscore in front of Set()?
1433ff Why not just move it to the next line? You have a multi-line
comment anyway.
2164 Bad division of lines.
2421 inseperable -> inseparable
2462 tese -> these
--------------
And now the more meaty remarks:
- There are a *lot* of coding style violations in there that have
already been mentioned to you before.
This is really wasting a lot of people's time (even several times),
and I really don't appreciate this at all.
I will refuse to review patches in the future in this case.
- Putting the merge/comparison logic into DelayedMessage is awkward.
It should should use a callback/interface mechanism for this (and only
provide convenience methods to simplify their implementations)
- DMScheduledMessage::SendMessage() fFailureCount += 10000, return -2?
- DelayedMessageSender::~DelayedMessageSender(): please use
wait_for_thread() for this; instead of writing
the exit message to the port, you could also just delete it, and let
the thread exit then.
- BColorMap is not a good name, as it leads you into a wrong direction.
If it's ui color specific, call it BUIColorMap, or something like
that.
- A send timeout of 25000 is a bit ridiculous - what are you trying to
achieve with this?
- BColorMap::SetUIColors(): since you own the message, you should
optimize for the normal case; your solution guarantees to provide
the worst possible performance for this, and even make the code more
complex than needed.
- BColorMap::Get(): why B_HOST_TO_BENDIAN_INT32()?
- CHECK_READ_STATUS/CHECK_WRITE_STATUS: we discourage the use of macros
for things like that.
- BColorMap::Set(const BMessage* message): same issue as SetUIColors(),
plus you can use SetInt32() instead of RemoveName()/AddInt32().
- B_FOLLOW_DEFAULT: why? And even more important: why in this patch?
It's already huge as it is, no point in making it even bigger.
And it's outdated API anyway, it'll be deprecated with R2.
Bye,
Axel.
Other related posts:
