llvm dev - Jul 2019 - RFC: changing variable naming rules in LLVM codebase

Rui Ueyama via llvm-dev <llvm-dev at lists.llvm.org> writes:
>  - LLVM's `/*foo=*/`-style comment to annotate function arguments need
>  to be handled automatically to make the tool scalable. So is the
>  doxygen @parameter.
This is a bit of a side note, but in my own work I've more recently
tried to move from this style:

foo.h

int foo(int a, bool doSomething);

foo.cpp

x = foo(a, /*doSomething=*/true);
y = foo(a, /*doSomething=*/false);

to something like:

foo.h

inline constexpr DoDoSomething   = true;
inline constexpr DontDoSomething = false;

int foo(int a, bool doSomething);

foo.cpp

x = foo(a, DoDoSomething);
y = foo(a, DontDoSomething);

One doesn't need the inline variable (and thus not C++17) if one uses
macros or enums or something else less elegant.

This kind of thing is slightly more cumbersome to do if the parameter
takes a wide range of values, but the most common place I see the former
style is for Boolean arguments.  Nevertheless, the wide-ranging case can
be handled:

bar.h

inline int Threshold(int x) { return x; }

int bar(int a, int threshold);

bar.cpp

x = bar(a, Threshold(1000));
y = bar(a, Threshold(100));

With either technique the "named values" could be wrapped in their own
namespaces to avoid collisions.

I wonder if there is any appetite for doing something similar in LLVM.

                       -David

Why would enums be less elegant than named boolean constants as you've
shown here?
http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html
(at
a random googling for "bool parameters considered harmful") reasonably
suggests splitting functions, but that's not always practical - often lots
of common code, etc (but writing wrappers isn't too bad in that case either
- two different functions with different names that both call the common
one with the boolean parameter - so it's not hard to trace from the call to
the implementation to know what that bool does, and is obvious by these two
side-by-side differently named functions) and the comments on that article
discuss enums as well.

On Fri, Jul 12, 2019 at 9:04 AM David Greene via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Rui Ueyama via llvm-dev <llvm-dev at lists.llvm.org> writes:
>
> >  - LLVM's `/*foo=*/`-style comment to annotate function arguments
need
> >  to be handled automatically to make the tool scalable. So is the
> >  doxygen @parameter.
>
> This is a bit of a side note, but in my own work I've more recently
> tried to move from this style:
>
> foo.h
>
> int foo(int a, bool doSomething);
>
> foo.cpp
>
> x = foo(a, /*doSomething=*/true);
> y = foo(a, /*doSomething=*/false);
>
> to something like:
>
> foo.h
>
> inline constexpr DoDoSomething   = true;
> inline constexpr DontDoSomething = false;
>
> int foo(int a, bool doSomething);
>
> foo.cpp
>
> x = foo(a, DoDoSomething);
> y = foo(a, DontDoSomething);
>
> One doesn't need the inline variable (and thus not C++17) if one uses
> macros or enums or something else less elegant.
>
> This kind of thing is slightly more cumbersome to do if the parameter
> takes a wide range of values, but the most common place I see the former
> style is for Boolean arguments.  Nevertheless, the wide-ranging case can
> be handled:
>
> bar.h
>
> inline int Threshold(int x) { return x; }
>
> int bar(int a, int threshold);
>
> bar.cpp
>
> x = bar(a, Threshold(1000));
> y = bar(a, Threshold(100));
>
> With either technique the "named values" could be wrapped in
their own
> namespaces to avoid collisions.
>
> I wonder if there is any appetite for doing something similar in LLVM.
>
>                        -David
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190712/1453f99f/attachment.html>

FWIW if I were writing that post today I'd probably say that doing
`foo(/*weak=*/true)` is often as good as `fooWeak()` *so long as* one
has tooling that checks and enforces the `/*weak=*/` comment is
present and correct.  The comment thing wasn't something I was aware
of back then, if it even existed.

In the specific case of "add weak observer" I still think that a
separate function is probably a better idea, but it's easy to come up
with examples where it's not, e.g. cases where you often have the
boolean as a variable already, and so you don't want to do `if (thing)
{ doX(); } else { do Y(); }` all the time.

On Fri, Jul 12, 2019 at 9:17 AM David Blaikie via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> Why would enums be less elegant than named boolean constants as you've
shown here?
http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html
(at a random googling for "bool parameters considered harmful")
reasonably suggests splitting functions, but that's not always practical -
often lots of common code, etc (but writing wrappers isn't too bad in that
case either - two different functions with different names that both call the
common one with the boolean parameter - so it's not hard to trace from the
call to the implementation to know what that bool does, and is obvious by these
two side-by-side differently named functions) and the comments on that article
discuss enums as well.
>
> On Fri, Jul 12, 2019 at 9:04 AM David Greene via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>>
>> Rui Ueyama via llvm-dev <llvm-dev at lists.llvm.org> writes:
>>
>> >  - LLVM's `/*foo=*/`-style comment to annotate function
arguments need
>> >  to be handled automatically to make the tool scalable. So is the
>> >  doxygen @parameter.
>>
>> This is a bit of a side note, but in my own work I've more recently
>> tried to move from this style:
>>
>> foo.h
>>
>> int foo(int a, bool doSomething);
>>
>> foo.cpp
>>
>> x = foo(a, /*doSomething=*/true);
>> y = foo(a, /*doSomething=*/false);
>>
>> to something like:
>>
>> foo.h
>>
>> inline constexpr DoDoSomething   = true;
>> inline constexpr DontDoSomething = false;
>>
>> int foo(int a, bool doSomething);
>>
>> foo.cpp
>>
>> x = foo(a, DoDoSomething);
>> y = foo(a, DontDoSomething);
>>
>> One doesn't need the inline variable (and thus not C++17) if one
uses
>> macros or enums or something else less elegant.
>>
>> This kind of thing is slightly more cumbersome to do if the parameter
>> takes a wide range of values, but the most common place I see the
former
>> style is for Boolean arguments.  Nevertheless, the wide-ranging case
can
>> be handled:
>>
>> bar.h
>>
>> inline int Threshold(int x) { return x; }
>>
>> int bar(int a, int threshold);
>>
>> bar.cpp
>>
>> x = bar(a, Threshold(1000));
>> y = bar(a, Threshold(100));
>>
>> With either technique the "named values" could be wrapped in
their own
>> namespaces to avoid collisions.
>>
>> I wonder if there is any appetite for doing something similar in LLVM.
>>
>>                        -David
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

David Blaikie <dblaikie at gmail.com> writes:
> Why would enums be less elegant than named boolean constants as you've
shown here?
Casting, mainly.  If the parameters were also changed to an enum type
that would be fine too, probably better than inline variables even.
>
http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html
> (at a random googling for "bool parameters considered harmful")
> reasonably suggests splitting functions, but that's not always
> practical - often lots of common code, etc (but writing wrappers isn't
> too bad in that case either - two different functions with different
> names that both call the common one with the boolean parameter - so
> it's not hard to trace from the call to the implementation to know
> what that bool does, and is obvious by these two side-by-side
> differently named functions) and the comments on that article discuss
> enums as well.
I like the two functions idea.  Of course scaling becomes a problem with
multiple Boolean parameters, as pointed out in the comments.

                           -David
> On Fri, Jul 12, 2019 at 9:04 AM David Greene via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>
>  Rui Ueyama via llvm-dev <llvm-dev at lists.llvm.org> writes:
>
>  >  - LLVM's `/*foo=*/`-style comment to annotate function arguments
need
>  >  to be handled automatically to make the tool scalable. So is the
>  >  doxygen @parameter.
>
>  This is a bit of a side note, but in my own work I've more recently
>  tried to move from this style:
>
>  foo.h
>
>  int foo(int a, bool doSomething);
>
>  foo.cpp
>
>  x = foo(a, /*doSomething=*/true);
>  y = foo(a, /*doSomething=*/false);
>
>  to something like:
>
>  foo.h
>
>  inline constexpr DoDoSomething   = true;
>  inline constexpr DontDoSomething = false;
>
>  int foo(int a, bool doSomething);
>
>  foo.cpp
>
>  x = foo(a, DoDoSomething);
>  y = foo(a, DontDoSomething);
>
>  One doesn't need the inline variable (and thus not C++17) if one uses
>  macros or enums or something else less elegant.
>
>  This kind of thing is slightly more cumbersome to do if the parameter
>  takes a wide range of values, but the most common place I see the former
>  style is for Boolean arguments.  Nevertheless, the wide-ranging case can
>  be handled:
>
>  bar.h
>
>  inline int Threshold(int x) { return x; }
>
>  int bar(int a, int threshold);
>
>  bar.cpp
>
>  x = bar(a, Threshold(1000));
>  y = bar(a, Threshold(100));
>
>  With either technique the "named values" could be wrapped in
their own
>  namespaces to avoid collisions.
>
>  I wonder if there is any appetite for doing something similar in LLVM.
>
>                         -David
>  _______________________________________________
>  LLVM Developers mailing list
>  llvm-dev at lists.llvm.org
>  https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev