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

Jason Wilkins jason.a.wilkins at gmail.com
Wed Oct 17 16:26:45 CEST 2012


If a person is allowed to make the case that somebody should use a
different style as long as they are polite and open minded then I also
see no problem.

On Wed, Oct 17, 2012 at 2:10 AM, Sergey Sharybin <sergey.vfx at gmail.com> wrote:
> In areas where you're maintainer you could stick to rules you want
> (only generally defined rules are [1]). In other areas you'll just stick to
> style used in that areas. If there're already mixed style in some files,
> stick to style f functions you're working and talk to module maintainer to
> update stylistic there. Can not see the issue here.
>
> [1] http://wiki.blender.org/index.php/Dev:Doc/CodeStyle
>
> On Tue, Oct 16, 2012 at 10:03 PM, Jason Wilkins
> <jason.a.wilkins at gmail.com>wrote:
>
>> 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
>> _______________________________________________
>> Bf-committers mailing list
>> Bf-committers at blender.org
>> http://lists.blender.org/mailman/listinfo/bf-committers
>>
>
>
>
> --
> With best regards, Sergey Sharybin
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers


More information about the Bf-committers mailing list