<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Nov 4, 2013 at 11:22 AM, Jon Eyolfson <span dir="ltr"><<a href="mailto:jon@eyolfson.com" target="_blank">jon@eyolfson.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> writes:<br>
<br>
> 2013/11/4 Jon Eyolfson <<a href="mailto:jon@eyolfson.com">jon@eyolfson.com</a>><br>
><br>
>><br>
>> Hello Clang Developers,<br>
>><br>
>> In my brief time with the clang codebase I've noticed instances of<br>
>> const_cast<br>
>> and mutable I don't believe are necessary. Are the uses of const_cast and<br>
>> mutable just temporary to avoid some compile-time type errors?<br>
><br>
><br>
> Some might be temporary, there are varying levels of 'technical debt' that<br>
> are acceptable/accrued/historical. Some might be deliberate/acceptable.<br>
> It's hard to say generally.<br>
><br>
><br>
>> Does const have<br>
>> a different meaning for internal clang code?<br>
><br>
><br>
> Not especially, though there are perhaps some quirks. One is that if an<br>
> class represents immutable objects then we may not bother using 'const' at<br>
> all, since all instances will always be const, we just drop the 'const'<br>
> from all instances. (see the llvm::Type hierarchy for an example of that)<br>
<br>
</div>That's a good point, although const wouldn't really hurt either.<br></blockquote><div><br></div><div>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. </div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">><br>
><br>
>> Is developer time prioritized<br>
>> elsewhere and you're looking for contributors to help with<br>
>> const-correctness?<br>
>><br>
><br>
> Possibly.<br>
><br>
><br>
>><br>
>> The attached example diff demonstrates what I believe is a const-correct<br>
>> version<br>
>> of CFGBlock::getTerminatorCondition(). I don't understand the reasoning<br>
>> behind<br>
>> including a non-const version of this member function that may<br>
>> unintentionally<br>
>> allow bad client code.<br>
>><br>
><br>
> What bad client code do you think this might unintentionally allow?<br>
<br>
</div>It allows clients to get a Stmt*, which could be used to modify the Stmt<br>
(although looking at the Doxygen, Stmt mostly has const member functions). It<br>
may be an issue if they use children() and modify something they shouldn't.<br></blockquote><div><br></div><div>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).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">><br>
> I believe this code just provided a const and non-const overload for the<br>
> convenience of users (arguable as to whether that's necessary at all, but<br>
> evidently no one relies on it at the moment if your code change compiles).<br>
> It just happens to be easier to implement the overloads by having one<br>
> overload defer to the other.<br>
><br>
> Personally I probably would've had the non-const overload defer to the<br>
> const overload (and const_cast the result) as that would be more correct<br>
> than possibly casting away const on an actually const object to call a<br>
> non-const member function from a const member function.<br>
<br>
</div>What's the reason for the non-const overload at this point? It seems to me that<br>
all AST nodes returned by any analysis should always be const. For me, if I<br>
wrote "Stmt* S = block->getTerminatorCondition();", I'd rather get a compiler<br>
error that it should be "const Stmt* S" then have a const_cast that I personally<br>
don't expect.<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks for the reply,<br>
Jon<br>
<div class="HOEnZb"><div class="h5">><br>
><br>
>><br>
>> After applying the diff, clang and the default plugins compile<br>
>> successfully as<br>
>> expected. Granted I don't have any other client code, so I can't see their<br>
>> uses<br>
>> (or misuses). I would argue that bad client code would now get a helpful<br>
>> error<br>
>> message instead of possibly really breaking something in the future.<br>
>> Clients can<br>
>> either fix their code or be explicit about breaking const and include the<br>
>> const_cast themselves.<br>
>><br>
>> This is my first email to a mailing list, so I apologize in advance for<br>
>> any misuse.<br>
>><br>
>> Jon<br>
>><br>
>> _______________________________________________<br>
>> cfe-dev mailing list<br>
>> <a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
>><br>
>><br>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
</div></div></blockquote></div><br></div></div>