[Bf-committers] Not adding #defined code in sources please

[Jason Wilkins]

I kind of like the "UNLIKELY" macro because it also seems like it is
self-documenting.  Sometimes I add checks and then comment them
PARANOIA because I know some ridiculous corner case that will happen
as soon as I don't check for it ;-)

On Tue, Jan 29, 2013 at 6:51 PM, Campbell Barton <ideasman42 at gmail.com> wrote:
> Some notes on this topic,
>
> When checking for ugly #defines, a few were in newly added code (and
> no, I didnt write them all :) ).
> So someone could go over and convert to functions at some point.
>
> Regarding LIKELY/UNLIKELY use, agree they shouldn't be all over the code.
> But for functions which are called often and have checks which are
> really unlikely, I think this is reasonable.
>
> Examples - a matrix failing to invert, a check isnan, isfinite, a
> normal failing to normalize.
>
> When writing this code you could add some comment
> if (isnan(f) == false) {  /* this almost _never_ happens */
>
> ... or you could use
> if (UNLIKELY(isfinite(f) == false)) {
>
> If the compiler uses this to make some optimization with branch
> prediction - great, otherwise its still hint to the developer that its
> an rare occurrence (in this case its obvious, but thats not always the
> case).
>
> On Mon, Jan 28, 2013 at 9:58 PM, Ton Roosendaal <ton at blender.org> wrote:
>> Hi,
>>
>> Yes the diff.txt is all fine.
>>
>> All I meant to say is that converting old code to use #defined code shouldn't be done. Also for new code we shouldn't do it.
>>
>> The fact Blender already has bad pieces of code is not a reason to a) copy that or b) fix that.
>>
>> That comes back to the other mail I sent: just stay away from old bad code, unless there's a maintainer who intends to improve or replace its functionality.
>>
>> Very easy guideline, and I think everyone will be happy with it. :)
>>
>> -Ton-
>>
>> ------------------------------------------------------------------------
>> Ton Roosendaal  Blender Foundation   ton at blender.org    www.blender.org
>> Blender Institute   Entrepotdok 57A  1018AD Amsterdam   The Netherlands
>>
>> On 28 Jan, 2013, at 4:36, Campbell Barton wrote:
>>
>>> On Fri, Jan 25, 2013 at 11:31 PM, Ton Roosendaal <ton at blender.org> wrote:
>>>> Hi,
>>>>
>>>> As part of our style guide, I propose to limit 'fake inlining' with #defines of code to an absolute minumum.
>>>> It makes debugging (breakpoints, code stepping) impossible, and makes code less readable.
>>>>
>>>> This is totally unnecessary for example:
>>>>
>>>> http://projects.blender.org/scm/viewvc.php/trunk/blender/source/blender/editors/sculpt_paint/paint_vertex.c?root=bf-blender&r1=53285&r2=53321
>>>
>>> Would you suggest this instead as being an improvement?
>>>
>>> http://www.graphicall.org/ftp/ideasman42/paintweight_use_static_func_diff.txt
>>>
>>> There are quite a few ugly defines in blenders code, try grep for...
>>>
>>> PLUGIN_DECLARE_SERIAL, ShowDeprecationWarning, BRICONTRGB,
>>> COLRAMP_GETPATH, BRUSH_CAPABILITY, DRAW_EM_MEASURE_STATS_FACEAREA,
>>> KEYFRAME_OK_CHECKS, HIGHLIGHT, ARRAY_SWAP, BLI_STR_UTF8_CPY,
>>> PUSH_HEAP_BODY, POP_HEAP_BODY, PASSATTRIB, SINGLETARGETNS_GET_TARS....
>>> more but you get the point.
>>>
>>> Would you say these are bad too?
>>>
>>> If so, should we do code cleanups to make out code less bad? :). Or do
>>> we invoke the "this is reliable production code rule" - even though
>>> the code is horrible to debug.
>>>
>>>
>>> There are factors which are tedious to discuss in mail...
>>> * Weather the macro makes calls to functions.
>>> * If the macro is likely to need debugging. VECCOPY is example where
>>> its not an issue so much.
>>> * If the macro makes use of ##name## in a way which would make not
>>> using a macro much more tricky...
>>> * If the macro ends up needing to have the entire namespace of the C
>>> function passed along to perform some simple operation as is the case
>>> with paint_vertex.c patch linked above.
>>>
>>> Ton - I strongly agree its good to limit use of macros as functions -
>>> but  there are exceptions to the rule... What is/isn't OK by your
>>> definition I'm still not sure on.
>>>
>>> I understand if you don't like long discussions about these topics
>>> (its really tedious!), but if you say my code is doing-it-wrong. I
>>> like to point out there are quite a few factors that lead to me making
>>> this choice and I think it was a reasonable one.
>>>
>>> Of course we can just put up with passing many variables around to
>>> static functions, or copying and pasting code about rather then using
>>> defines, its becomes a matter of personal preference at some point
>>> too.
>>>
>>>> Also defines like "LIKELY" or "UNLIKELY" should go away - unless in local code segments that require heavily optimizing.
>>>>
>>>> Adding inline functions should also be limited to cases where it's proven to be required and giving massive speedups. Optimizing in general shouldn't be needed ever. Good code design is always superior.
>>>>
>>>> So let's stop attempting to do code clean work now, unless approved in advance by the people who actively work on that code.
>>>>
>>>> Laters,
>>>>
>>>> -Ton-
>>>>
>>>> ------------------------------------------------------------------------
>>>> Ton Roosendaal  Blender Foundation   ton at blender.org    www.blender.org
>>>> Blender Institute   Entrepotdok 57A  1018AD Amsterdam   The Netherlands
>>>>
>>>> _______________________________________________
>>>> Bf-committers mailing list
>>>> Bf-committers at blender.org
>>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>>
>>>
>>>
>>> --
>>> - Campbell
>>> _______________________________________________
>>> Bf-committers mailing list
>>> Bf-committers at blender.org
>>> http://lists.blender.org/mailman/listinfo/bf-committers
>>
>> _______________________________________________
>> Bf-committers mailing list
>> Bf-committers at blender.org
>> http://lists.blender.org/mailman/listinfo/bf-committers
>
>
>
> --
> - Campbell
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers