On Thu, Aug 20, 2015 at 5:18 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
> On Wed, Aug 19, 2015 at 2:52 PM, Aaron Ballman <aa...@aaronballman.com>
> wrote:
>>
>> 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.
>
>
> I suspect this patch breaks Objective-C cases (I hadn't previously noticed
> that D was just a Decl*, not a FunctionDecl*). Since it's using a
> RecursiveASTVisitor, it's going to deserialize everything anyway, so we
> don't really gain from formulating it this way. Let's go with your original
> patch.
Yeah, I think it would break ObjC. I went with the original patch,
plus a test case, in r245616. Thanks!
~Aaron
>
>> ~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
>> >
>> >
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
