[cfe-commits] [PATCH] Add isConstMethod(), isVolatileMethod() to CXXMethodDecl

[David Blaikie]

On Wed, Aug 8, 2012 at 11:37 AM, Sam Panzer <panzer at google.com> wrote:
> On Tue, Aug 7, 2012 at 11:48 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>> >> On Tue, Aug 7, 2012 at 10:30 AM, Sam Panzer <panzer at google.com> wrote:
>> >> > Several places in the codebase determine if a method is const or
>> >> > volatile by
>> >> > querying the method's type qualifiers directly,
>>
>> Oh, sorry I meant to mention this in my previous email: could you
>> point to the locations that are already doing this manually? and
>> possibly even update them to use this new utility function so we can
>> see what kind of cleanup it provides?
>>
>
> That would make sense. I found a number of uses that I could fix directly,
> though some of them blindly check to see if a function's type is const
> qualified without checking to see if it's actually a member function. I
> moved isConst, isVolatile, and isRestrict into FunctionType and exposed them
> from CXXMethodDecl, after removing the assert again. One potential downside
> is that MethodDecl.getType().isConstQualified() will still not do what I
> originally expected.
>
> The new patch removes as many usages of directly checking Qualifiers bits as
> I thought I could change and removes one of the (at least) two different
> ways to do this check (i.e.
> Qualifiers::fromCVRMask(Method->getTypeQualifiers()).hasConst()) entirely.

Looks good to me except for the getAs calls in DeclCXX.h - those
should be castAs, by the looks of it.

http://clang.llvm.org/doxygen/classclang_1_1Type.html#ac4a3f2c505332c3c556bd20497d1a5c3

This method has the same relationship to getAs<T> as cast<T> has to
dyn_cast<T>; which is to say, the underlying type *must* have the
intended type, and this method will never return null.

(since you dereference the return value immediately, it'd better not be null)

Please commit with this change included.

In theory you could keep the "&& FT->getTypeQuals()" check in
DeclPrinter.cpp, but I tend to agree with you that it's probably
simpler not to. You could check the revision history to see if it was
added explicitly with a specific (perf, if anything) justification,
but I doubt it.

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