David Blaikie <dblaikie@gmail.com> writes:
>
>>
>> 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.