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

[Sam Panzer]

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



>
> 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 :)


>
> 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.
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120809/fbb7a88e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: const-method.patch
Type: application/octet-stream
Size: 6640 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120809/fbb7a88e/attachment.obj>