<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Aug 8, 2012 at 3:12 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.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"><br>
On Wed, Aug 8, 2012 at 11:37 AM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> wrote:<br>
> On Tue, Aug 7, 2012 at 11:48 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>><br>
>> >> On Tue, Aug 7, 2012 at 10:30 AM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> wrote:<br>
>> >> > Several places in the codebase determine if a method is const or<br>
>> >> > volatile by<br>
>> >> > querying the method's type qualifiers directly,<br>
>><br>
>> Oh, sorry I meant to mention this in my previous email: could you<br>
>> point to the locations that are already doing this manually? and<br>
>> possibly even update them to use this new utility function so we can<br>
>> see what kind of cleanup it provides?<br>
>><br>
><br>
> That would make sense. I found a number of uses that I could fix directly,<br>
> though some of them blindly check to see if a function's type is const<br>
> qualified without checking to see if it's actually a member function. I<br>
> moved isConst, isVolatile, and isRestrict into FunctionType and exposed them<br>
> from CXXMethodDecl, after removing the assert again. One potential downside<br>
> is that MethodDecl.getType().isConstQualified() will still not do what I<br>
> originally expected.<br>
><br>
> The new patch removes as many usages of directly checking Qualifiers bits as<br>
> I thought I could change and removes one of the (at least) two different<br>
> ways to do this check (i.e.<br>
> Qualifiers::fromCVRMask(Method->getTypeQualifiers()).hasConst()) entirely.<br>
<br>
</div>Looks good to me except for the getAs calls in DeclCXX.h - those<br>
should be castAs, by the looks of it.<br>
<br></blockquote><div><br></div><div>Yep, adjusted to castAs.</div><div>Just for fun, I ran a grep for getAs() followed immediately by a method call, which turned up 295 cases that should probably be castAs(). If this sort of thing should be cleaned up... </div>
<div>$ git grep -E '(->|\.)getAs<\w+>\(\)->' | wc -l</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<a href="http://clang.llvm.org/doxygen/classclang_1_1Type.html#ac4a3f2c505332c3c556bd20497d1a5c3" target="_blank">http://clang.llvm.org/doxygen/classclang_1_1Type.html#ac4a3f2c505332c3c556bd20497d1a5c3</a><br>
<br>
This method has the same relationship to getAs<T> as cast<T> has to<br>
dyn_cast<T>; which is to say, the underlying type *must* have the<br>
intended type, and this method will never return null.<br>
<br>
(since you dereference the return value immediately, it'd better not be null)<br>
<br>
Please commit with this change included.<br></blockquote><div><br></div><div>I don't have commit access, so here's a patch instead :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
In theory you could keep the "&& FT->getTypeQuals()" check in<br>
DeclPrinter.cpp, but I tend to agree with you that it's probably<br>
simpler not to. You could check the revision history to see if it was<br>
added explicitly with a specific (perf, if anything) justification,<br>
but I doubt it.<br></blockquote><div><br></div><div>That's just how the code was written, back in 2010, so I don't think it was for a particular reason.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class=""><div class="h5"><br>
>> >> > but it makes sense to<br>
>> >> > centralize the (simple) logic directly in CXXMethodDecl. It's<br>
>> >> > particularly<br>
>> >> > useful for refactoring tools :)<br>
>> >><br>
>> >> Given that they're already members on CXXMethodDecl, the "Method" in<br>
>> >> the name seems a bit redundant (I'm not dead set on this, though - I<br>
>> >> appreciate that "isConst" might be a bit more unclear than the<br>
>> >> existing things like "isVirtual", etc) would "isConst" and<br>
>> >> "isVolatile" suffice?<br>
>> ><br>
>> ><br>
>> > I considered "isConst" and "isConstQualified" along with "isConstMethod"<br>
>> > -<br>
>> > mostly to distinguish between type qualifier const and method qualifier<br>
>> > const. I don't think "isConst" is unclear so long as it is accessed from<br>
>> > a<br>
>> > well-named variable.<br>
>> ><br>
>> >><br>
>> >><br>
>> >> Also how do these methods fail if the method is a static member<br>
>> >> function? Would it be helpful to have an assert there to ensure these<br>
>> >> are only called on non-static member functions?<br>
>> ><br>
>> ><br>
>> > Yes! Asserts added.<br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>