On Wed, Aug 8, 2012 at 3:12 PM, David Blaikie <[email protected]> wrote:
> > On Wed, Aug 8, 2012 at 11:37 AM, Sam Panzer <[email protected]> wrote: > > On Tue, Aug 7, 2012 at 11:48 AM, David Blaikie <[email protected]> > wrote: > >> > >> > >> >> On Tue, Aug 7, 2012 at 10:30 AM, Sam Panzer <[email protected]> > 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. > >> > > > > > >
const-method.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
