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

[David Blaikie]

On Thu, Aug 9, 2012 at 4:44 PM, Sam Panzer <panzer at google.com> wrote:
>
>
>
> On Wed, Aug 8, 2012 at 3:12 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>> 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.
>>
>
> Yep, adjusted to castAs.
> 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...
> $ git grep -E '(->|\.)getAs<\w+>\(\)->' | wc -l

Yeah, that might be something to tidy up...

>
>
>>
>>
>> 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.
>
>
> I don't have commit access, so here's a patch instead :)

Looks good. Committed as r161647.

Side note: if you create a ~/.gitconfig with:

  [diff]
    noprefix = true

Your patches won't have the a/b prefixes in the diffs & people will be
able to apply them with patch -p0 as they would with svn patches.

>
>>
>>
>> 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.
>
>
> That's just how the code was written, back in 2010, so I don't think it was
> for a particular reason.
>
>>
>>
>> >> >> > 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.
>> >> >
>> >
>> >
>
>