On Wed, Aug 19, 2015 at 5:23 PM, Richard Smith <rich...@metafoo.co.uk> wrote: > It looks like this would only happen for a late-parsed template that the > analysis code is checking before it is parsed. Should we really be running > these checks at all in that case?
This code is being called from DataRecursiveASTVisitor (using an AnalysisContext) on generic Decl objects to determine whether we *should* run a checker or not. I think it's reasonable, but am not overly familiar with this part of the code. > Also, it looks like this code doesn't actually want the body at all, and > just wants to get the location of the definition. Asking for the body here > does not seem like the right approach, because it'll cause the external AST > source (if there is one) to fault the body in. > > The right fix is probably something like: > > const FunctionDecl *Def; > SourceLocation SL = (D->isDefined(Def) ? Def : D)->getLocation(); I've attached an updated patch with this suggested approach. It appears to work nicely. If you like the approach, I'll see what I can come up with for a test case and commit. ~Aaron > > On Wed, Aug 19, 2015 at 1:44 PM, Aaron Ballman <aa...@aaronballman.com> > wrote: >> >> The attached patch resolves the null pointer crash I am seeing. >> >> ~Aaron >> >> On Wed, Aug 19, 2015 at 1:24 PM, Aaron Ballman <aa...@aaronballman.com> >> wrote: >> > On Wed, Aug 19, 2015 at 11:33 AM, Aaron Ballman <aa...@aaronballman.com> >> > wrote: >> >> When I run the following test code through clang-tidy -checks=*, I get >> >> a crash from some relatively strange behavior with FunctionDecl. >> >> >> >> template <class T> struct remove_reference {typedef T type;}; >> >> template <class T> struct remove_reference<T&> {typedef T type;}; >> >> template <class T> struct remove_reference<T&&> {typedef T type;}; >> >> >> >> template <typename T> >> >> typename remove_reference<T>::type&& move(T&& arg) { >> >> return static_cast<typename remove_reference<T>::type&&>(arg); >> >> } >> >> >> >> AnalysisConsumer::getModeForDecl() is called, and it has code that >> >> does: D->hasBody() ? D->getBody()->stuff : stuff; >> >> >> >> What's strange is that hasBody() returns true, but getBody() returns >> >> nullptr. I don't think that this should be possible. I'm wondering >> >> whether it's purposeful that hasBody() does not check >> >> Definition->Body? >> > >> > Looking into this a bit further, the issue is that hasBody() checks >> > for Definition->Body *or* Definition->IsLateTemplateParsed when >> > deciding to return true. In my case, Body is null, but >> > IsLateTemplateParsed is true, so hasBody() returns true. However, >> > getBody() doesn't have any special logic for late template parsed >> > function bodies. >> > >> >> Also, the FunctionDecl in question is for move(), which does have a >> >> body, so I do not understand why the definition would claim there is >> >> no body. >> >> >> >> Ideas, or should I file a bug report? >> > >> > From my further look, I wonder if the correct solution to this is to >> > simply call getBody() and handle a nullptr return instead of calling >> > hasBody() first. It basically makes the check to ignore late parsed >> > template bodies an implicit one. >> > >> > ~Aaron > >
AnalysisConsumer.cpp.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits