On 10/09/2010 08:59 PM, Zabb65 wrote:
> I ran it through hg blame and it claims that revision 11977 was when a
> big block in pipeline.cpp changed
> [...]
> http://hg.secondlife.com/viewer-development/changeset/c09f9bcd9d20
> however does not show this, and I suspect that it is heavily truncated
> on bitbucket, even though the full patch can be pulled from the hg
> repository itself.
I can't see it in the raw changeset, either. Probably |hg blame| got
confused by the huge merges and them being backed out and un-backed out
('backed in'?) again. The revision where this really changed seems to be
83606bbdc084
<http://hg.secondlife.com/viewer-development/changeset/83606bbdc084>.
Found via hg bisect, like this:
hg bisect -g 0
hg bisect -b 467c72e14d6e
hg bisect -c'grep "(mRenderTypeMask & (1<<type))) ? TRUE : FALSE;"
indra/newview/pipeline.h'
(467c72e14d6e is the big merge that got backed out in 11977.)
> I tend to agree with Argent on this, it should have a comment if there
> is a hack in place to make it all function.
> [...]
> Comments for hacks like this are a good thing. Leaving them only in
> commit messages leaves them to be lost and forgotten and then broken
> by whoever browses the code.
Yeah, this is definitely something that needs in-code documentation.
> On Sat, Oct 9, 2010 at 12:19, Argent <secret.arg...@gmail.com> wrote:
>> I don't normally gripe about stuff like this, but somehow this one
>> triggered my twitches from 20 years of supporting PhD programmers.
>> Brilliant guys, but sometimes it's SO hard to figure out what they're
>> trying to do.
>>
>> This is a case where the trinary "?:" operator is much more readable
>> and understandable.
>>
>> (type == 0) ? FALSE : mRenderTypeEnabled[type];
>>
>> But even better:
>>
>> if(type == 0)
>> return FALSE; // explain why here .. eg "in this context we are
>> always rendering attached prims on the head when blah blah..."
>> else
>> return mRenderTypeEnabled[type];
>>
Maybe it's because I'm in academia myself, but I don't think these are
significantly more readable than Merov's line. The real issue is IMHO
the lack of comments. Of course, the original implementation
<http://hg.secondlife.com/viewer-development/src/9d45e31e4e39/indra/newview/pipeline.h#cl-281>
was rather confusing. Implicit casting to bool is commonly used for null
pointer checks, but (without wanting to blame anyone) can probably
safely be declared bad style for non-pointer variables, comments being
present or not.
Maybe a stupid question, but why do we need special casing here at all?
Can't we just set |mRenderTypeEnabled[0]| to |FALSE| at an appropriate
place? (Of course with a comment there explaining why we do that.)
Cheers,
Boroondas
_______________________________________________
Policies and (un)subscribe information available here:
http://wiki.secondlife.com/wiki/OpenSource-Dev
Please read the policies before posting to keep unmoderated posting privileges
