[Bf-committers] Using 'const' on primitive function arguments passed by value (Please don't do this)

Tue Oct 16 22:03:05 CEST 2012

My main problem was the inconsistency and the fact that my intuition
led me astray because what I was seeing was non-idiomatic.  If the
inconsistencies could be ironed out then highly verbose declarations
could/should probably be tolerated (I guess *grudge*).  I realize
these kinds of inscrutable declarations actually happen all the time
in library code now that I think about it more.  Documentation may be
the only place I can realistically expect somebody to keep things
simple.  Unfortunately, the only documentation most of this stuff has
is the code itself (hopefully automatic documentation generation could
scrape all the extra crap off, and consider it for "the compiler's
eyes only").

Why does C still require separate header files?... It isn't 1973 anymore ;-)

On Sun, Oct 14, 2012 at 9:34 PM, Campbell Barton <ideasman42 at gmail.com> wrote:
> @patrick, this isn't really a good option IMHO,
> the reason this is useful is that you can be sure the value wont
> change under normal conditions when assignment isnt so obvious (in a
> macro for eg), which you wont get with a naming convention,
> Possibly if we had all developers full-time and have them agree on
> conventions - but with merging GSOC branches and having patches
> accepted from less involved developers, its hard to make sure they
> follow loose conventions like this.
> If someone sets a 'int c_blah' in a function, it could slip through in
> a merge and its likely nobody would notice.
>
> I don't really see the big problem with having const in function declarations,
> if you start to get picky with this stuff in C - there are many other
> things you could complain about that are very cumbersome. like having
> attributes for nonull or unused result.
>
> eg.
>
> void *MEM_callocN(size_t len, const char *str)
> #if MEM_GNU_ATTRIBUTES
> __attribute__((warn_unused_result))
> __attribute__((nonnull(2)))
> __attribute__((alloc_size(1)))
> #endif
>
> having compilers (gcc and clang in this case) know this info is very
> useful and can help track real errors in code, but its not pretty.
> in these cases I'm happy to choose verbose&safe declarations over brief ones.
>
> On Mon, Oct 15, 2012 at 11:45 AM, patrick boelens <p_boelens at msn.com> wrote:
>>
>> It's been a while since I've read the initial mails on this, but if I remember correctly the main reason for using foo(const int bar) was as a reminder that bar shouldn't change. In that case, maybe a nice middle ground would be to use a var naming convention instead (i.e.: foo(int c_bar) or foo(int const_bar))? That way we can forego the discussion on whether or not such uses of const are warranted, warnings for inconsistent use would be prevented, and the indiciation is still there. I have no idea if this is considered ugly(er), but just thought I'd throw it out there.
>>
>> -Patrick
>>
>>> Date: Sun, 14 Oct 2012 18:37:12 -0500
>>> From: jason.a.wilkins at gmail.com
>>> To: bf-committers at blender.org
>>> Subject: Re: [Bf-committers] Using 'const' on primitive function arguments passed by value (Please don't do this)
>>>
>>> Well that's unfortunate because that would have been acceptable to me.
>>>
>>> On Wed, Oct 10, 2012 at 11:03 PM, Campbell Barton <ideasman42 at gmail.com> wrote:
>>> > Checked, and that would work in MSVC since its the same warning that
>>> > checks a function matches its declaration in the header,
>>> > so turning that off isn't an option (was thinking it may have been a
>>> > different warning ID).
>>> >
>>> >
>>> > On Thu, Oct 11, 2012 at 12:45 AM, Campbell Barton <ideasman42 at gmail.com> wrote:
>>> >> @Jason, wouldn't it work to remove offending const from headers and
>>> >> disable the warning in MSVC?
>>> >>
>>> >> On Wed, Oct 10, 2012 at 9:57 PM, Jason Wilkins
>>> >> <jason.a.wilkins at gmail.com> wrote:
>>> >>> C is ALWAYS call by value and C++ is call by value by default there is
>>> >>> no need to mark the parameters const to know it will not change the
>>> >>> caller's value in that case.  The const on a call by value function
>>> >>> parameter is an internal detail of the function itself.  The function
>>> >>> is saying that it won't change its copy.  To the caller this is like
>>> >>> function saying what kind of magazines are hidden under its mattress.
>>> >>>
>>> >>> I do agree that C# is nice in that you have to mark a parameter as
>>> >>> 'ref' both in the interface and at each call site (same with having to
>>> >>> declare a shadowing variable as 'shadow').  However imagine if you had
>>> >>> to prefix 'val' every time you called the function even though that is
>>> >>> the default.  That would be very ugly and unneeded.  That sort of
>>> >>> thing would be similar to what I'm arguing against here.
>>> >>>
>>> >>> I've made my case though, so if people disagree I'll just have to live
>>> >>> with their code.  I won't belabor the point any more.
>>> >>>
>>> >>> On Tue, Oct 9, 2012 at 3:02 PM, Dahlia Trimble <dahliatrimble at gmail.com> wrote:
>>> >>>> I don't poke around much in blender source but I've found in c++ in general
>>> >>>> that I kind of like being able to declare a function parameter as const,
>>> >>>> for the simple reason that I don't see well and it helps me easily
>>> >>>> differentiate foo(const int bar) from foo(int& bar). Where in the latter I
>>> >>>> might not notice it's a reference.
>>> >>>>
>>> >>>> I like how C# handles references: if you declare a function you need to use
>>> >>>> the ref keyword both in the declaration and when calling the function. This
>>> >>>> way you cannot call a function that may modify a passed parameter without
>>> >>>> explicitly stating that you are aware it's passed by reference. I think c++
>>> >>>> could use something similar.
>>> >>>>
>>> >>>>
>>> >>>> On Sun, Oct 7, 2012 at 10:17 PM, Jason Wilkins <jason.a.wilkins at gmail.com>wrote:
>>> >>>>
>>> >>>>> OK, when I get warnings I guess I will fix them by adding the const in
>>> >>>>> the appropriate spot.  At least this kind of const is not viral :)
>>> >>>>>
>>> >>>>> On Sun, Oct 7, 2012 at 9:55 AM, Campbell Barton <ideasman42 at gmail.com>
>>> >>>>> wrote:
>>> >>>>> > On Mon, Oct 8, 2012 at 1:26 AM, Jason Wilkins <jason.a.wilkins at gmail.com>
>>> >>>>> wrote:
>>> >>>>> >> I probably would not notice except that it is done inconsistently and
>>> >>>>> >> I get lots of warnings.  I tend to fix things by removing the const
>>> >>>>> >> though.
>>> >>>>> >>
>>> >>>>> >> Another thing I noticed was returning a const pointer from a function
>>> >>>>> >> but then expecting to free it using 'free'.  Dynamically allocated
>>> >>>>> >> memory is not 'const' for the purposes of 'free'.  It would probably
>>> >>>>> >> be better to cast away the const inside a special function instead of
>>> >>>>> >> asking the user to use a raw 'free'.  (or just not use const).
>>> >>>>> > agree, const can be a bit of a pain like this.
>>> >>>>> >
>>> >>>>> >> I'm all for using const, but I really question if making primitive
>>> >>>>> >> arguments const is more trouble than it is worth. I mean, to follow
>>> >>>>> >> through on it would be a huge task.  It only generates a warning (no
>>> >>>>> >> error) when done inconsistently and if you change your mind now you
>>> >>>>> >> have to make a change in two places instead of one.
>>> >>>>> >
>>> >>>>> > You dont need to follow it though, if a dev wants to use it, they can,
>>> >>>>> > when done inconsistently it wont give warnings in GCC - but this stuff
>>> >>>>> > is really issue with multi-platform dev, it happens in other areas too
>>> >>>>> > - I often wake up to find blender wont compile because of an error in
>>> >>>>> > some commit from a dev with a different environment (-Werror helps
>>> >>>>> > here too :) ).
>>> >>>>> > you can correct warnings, or mail some other dev to fix, last I
>>> >>>>> > compiled on MSVC I didnt see any warnings like this though (a few
>>> >>>>> > weeks back).
>>> >>>>> >
>>> >>>>> >> To me this is a C programmers version of those people who have to turn
>>> >>>>> >> a light switch on and off a prime number of times.
>>> >>>>> >>
>>> >>>>> >> Reason #5 would be that it is just cluttered and ugly.  It decreases
>>> >>>>> >> readability instead of enhancing it.  It reminds me of when I had a
>>> >>>>> >> phase where I wanted to add 'struct' to everything so that people knew
>>> >>>>> >> that, yes, this is a struct.
>>> >>>>> >
>>> >>>>> > think this comes down to personal preference, if you think its not
>>> >>>>> > warranted, don't add it to your code.
>>> >>>>> >
>>> >>>>> >> I guess detecting stack corruption does not seem like a plus to me
>>> >>>>> >> because my environment does this very aggressively without help
>>> >>>>> >> (MSVC).
>>> >>>>> >
>>> >>>>> > Its no protection against stack corruption,
>>> >>>>> > It just means you know the var wont change under normal conditions, if
>>> >>>>> > it does change that something exceptional/wrong is happening.
>>> >>>>> >
>>> >>>>> >> On Sun, Oct 7, 2012 at 8:22 AM, Campbell Barton <ideasman42 at gmail.com>
>>> >>>>> wrote:
>>> >>>>> >>> On Sun, Oct 7, 2012 at 8:25 PM, Jason Wilkins <
>>> >>>>> jason.a.wilkins at gmail.com> wrote:
>>> >>>>> >>>> If I had a function with the prototype: foo(int bar)
>>> >>>>> >>>>
>>> >>>>> >>>> It may be tempting to declare the it as: foo(const int bar)
>>> >>>>> >>>>
>>> >>>>> >>>> The reason would be that bar is not modified inside of foo, so by
>>> >>>>> >>>> declaring it this way we prevent ourselves from accidentally modifying
>>> >>>>> >>>> it.
>>> >>>>> >>>>
>>> >>>>> >>>> This is not idiomatic C, and for good reasons.
>>> >>>>> >>>>
>>> >>>>> >>>> 1) We use 'const' on pointers to indicate that we are not going to
>>> >>>>> >>>> modify what is pointed at, when a programmer sees 'const int' it is
>>> >>>>> >>>> momentarily confusing because we expect 'const int*'
>>> >>>>> >>>>
>>> >>>>> >>>> 2) This exposes internal details of the function to the outside world.
>>> >>>>> >>>>  The fact that 'bar' is const in this case is not actually a part of
>>> >>>>> >>>> the interface of that function.
>>> >>>>> >>>>
>>> >>>>> >>>> 3) If we change our minds later and actually do want to modify the
>>> >>>>> >>>> copy of 'bar' inside the function then we have to change the interface
>>> >>>>> >>>> again, but as per #2 it actually has nothing to do with the user of
>>> >>>>> >>>> 'foo'
>>> >>>>> >>>>
>>> >>>>> >>>> 4) It is just not idiomatic.  Looking at it is like listening to a
>>> >>>>> >>>> foreigner speak your native language in "creative" ways.
>>> >>>>> >>>>
>>> >>>>> >>>> I have not figured out who is doing this, but please stop :)
>>> >>>>> >>>
>>> >>>>> >>> I've been doing this and Im not convinced its a bad thing, in some
>>> >>>>> >>> functions its a good hint that a var is `fixed` and shouldn't be
>>> >>>>> >>> changed.
>>> >>>>> >>> If a dev wants to change it they can just remove the `const` but it
>>> >>>>> >>> means they think twice before doing it (as in - maybe there is a good
>>> >>>>> >>> reason it shouldn't be changed).
>>> >>>>> >>>
>>> >>>>> >>> The main reason I like to have this sometimes is when debugging you
>>> >>>>> >>> know for sure a var wont change, if it does - its a buffer overflow or
>>> >>>>> >>> something exceptional.
>>> >>>>> >>> Often its not really an issue - but there are cases it can help verify
>>> >>>>> >>> whats going on when reading the function.
>>> >>>>> >>>
>>> >>>>> >>> That the `const` gets in the header is a little inconvenience if it
>>> >>>>> >>> changes often - but IMHO changing those is rare enough that its not an
>>> >>>>> >>> issue.
>>> >>>>> >>> _______________________________________________
>>> >>>>> >>> 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
>>> >>>>> _______________________________________________
>>> >>>>> 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
>>> >>> _______________________________________________
>>> >>> Bf-committers mailing list
>>> >>> Bf-committers at blender.org
>>> >>> http://lists.blender.org/mailman/listinfo/bf-committers
>>> >>
>>> >>
>>> >>
>>> >> --
>>> >> - Campbell
>>> >
>>> >
>>> >
>>> > --
>>> > - 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
>>
>> _______________________________________________
>> 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