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

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

On Fri, Jul 12, 2019 at 10:31 AM David Greene <dag at cray.com> wrote:
> 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.
>
Oh, yeah, I meant changing the parameter type to a custom enum - forcing
users to have to use the more legible enum names rather than the
inscrutible boolean constants.

>
> >
>
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190712/4fdb4dc3/attachment.html>

On Sat, Jul 13, 2019 at 4:13 AM David Blaikie via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
>
> On Fri, Jul 12, 2019 at 10:31 AM David Greene <dag at cray.com>
wrote:
>
>> 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.
>>
>
> Oh, yeah, I meant changing the parameter type to a custom enum - forcing
> users to have to use the more legible enum names rather than the
> inscrutible boolean constants.
>
FWIW, I found that clang-tidy can fix up /*foo=*/-style comments. I ran the
tool and submitted it as https://reviews.llvm.org/rL366177.

>
>>
>> >
>>
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
>>
> _______________________________________________
> 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/20190716/a378c6e4/attachment.html>