[cfe-dev] Question regarding const-correctness

[David Blaikie]

On Mon, Nov 4, 2013 at 11:22 AM, Jon Eyolfson <jon at eyolfson.com> wrote:

> David Blaikie <dblaikie at gmail.com> writes:
>
> > 2013/11/4 Jon Eyolfson <jon at eyolfson.com>
> >
> >>
> >> Hello Clang Developers,
> >>
> >> In my brief time with the clang codebase I've noticed instances of
> >> const_cast
> >> and mutable I don't believe are necessary. Are the uses of const_cast
> and
> >> mutable just temporary to avoid some compile-time type errors?
> >
> >
> > Some might be temporary, there are varying levels of 'technical debt'
> that
> > are acceptable/accrued/historical. Some might be deliberate/acceptable.
> > It's hard to say generally.
> >
> >
> >> Does const have
> >> a different meaning for internal clang code?
> >
> >
> > Not especially, though there are perhaps some quirks. One is that if an
> > class represents immutable objects then we may not bother using 'const'
> at
> > all, since all instances will always be const, we just drop the 'const'
> > from all instances. (see the llvm::Type hierarchy for an example of that)
>
> That's a good point, although const wouldn't really hurt either.
>

Some people (by some people I mean the lead of the project, Chris Lattner)
find the const to be just extra textual noise/volume and prefer not to
include it for immutable types.


> >
> >
> >> Is developer time prioritized
> >> elsewhere and you're looking for contributors to help with
> >> const-correctness?
> >>
> >
> > Possibly.
> >
> >
> >>
> >> The attached example diff demonstrates what I believe is a const-correct
> >> version
> >> of CFGBlock::getTerminatorCondition(). I don't understand the reasoning
> >> behind
> >> including a non-const version of this member function that may
> >> unintentionally
> >> allow bad client code.
> >>
> >
> > What bad client code do you think this might unintentionally allow?
>
> It allows clients to get a Stmt*, which could be used to modify the Stmt
> (although looking at the Doxygen, Stmt mostly has const member functions).
> It
> may be an issue if they use children() and modify something they shouldn't.
>

That assumes they shouldn't modify them - yes, I suppose certain
modifications would invalidate the CFG, but that's not necessarily the
CFG's responsibility to ensure that. I haven't looked at the API design
enough to know (as a rule, the AST is not mutated, but there are some
narrow cases where it is, I believe). Evidently in this particular case no
client needed to mutate the Stmt at the moment, but without looking more
broadly I wouldn't assume that's never the case (it might even be valuable
to look at what commit introduced the const/non-const overloads and whether
there was a use-case at some point for the non-const overload).


> >
> > I believe this code just provided a const and non-const overload for the
> > convenience of users (arguable as to whether that's necessary at all, but
> > evidently no one relies on it at the moment if your code change
> compiles).
> > It just happens to be easier to implement the overloads by having one
> > overload defer to the other.
> >
> > Personally I probably would've had the non-const overload defer to the
> > const overload (and const_cast the result) as that would be more correct
> > than possibly casting away const on an actually const object to call a
> > non-const member function from a const member function.
>
> What's the reason for the non-const overload at this point? It seems to me
> that
> all AST nodes returned by any analysis should always be const. For me, if I
> wrote "Stmt* S = block->getTerminatorCondition();", I'd rather get a
> compiler
> error that it should be "const Stmt* S" then have a const_cast that I
> personally
> don't expect.
>

There is no 'real' const_cast, though - the const_cast is just an
implementation detail used to avoid duplicating the implementation for
const and non-const. Note that CFG already has non-const Stmt pointers that
it could just return without any const_casting.


>
> Thanks for the reply,
> Jon
> >
> >
> >>
> >> After applying the diff, clang and the default plugins compile
> >> successfully as
> >> expected. Granted I don't have any other client code, so I can't see
> their
> >> uses
> >> (or misuses). I would argue that bad client code would now get a helpful
> >> error
> >> message instead of possibly really breaking something in the future.
> >> Clients can
> >> either fix their code or be explicit about breaking const and include
> the
> >> const_cast themselves.
> >>
> >> This is my first email to a mailing list, so I apologize in advance for
> >> any misuse.
> >>
> >> Jon
> >>
> >> _______________________________________________
> >> cfe-dev mailing list
> >> cfe-dev at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
> >>
> >>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20131104/90e1f843/attachment.html>