On Thu, Aug 9, 2012 at 4:44 PM, Sam Panzer <[email protected]> wrote: > > > > 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
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. >> >> > >> > >> > > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
