[patch] Don't let invalid methods mark their RecordDecl invalid

[John McCall]

On Dec 20, 2013, at 4:27 PM, Nico Weber <thakis at chromium.org> wrote:
> On Fri, Dec 20, 2013 at 4:18 PM, John McCall <rjmccall at apple.com> wrote:
> On Dec 20, 2013, at 4:06 PM, Reid Kleckner <rnk at google.com> wrote:
>> +John, since it was his code.
>> 
>> Are there other ways we can get an invalid record decl and then do layout on a derived class to trigger PR18284?  If so, it seems like this doesn't fix the underlying issue.
> 
> A class should definitely be invalid if any of its base classes is invalid.  Making it invalid when one of its members is invalid is probably for the best; I would guess that that’s now occurring through some other channel, causing the original test to successfully not crash.
> 
> As far as I can tell, classes aren't always marked invalid if they have invalid methods at the moment. One example from PR18284 where RunTest is an invalid method but both B and A are valid:
> 
>   class A {};
>   class C : public A {};
>   void A::RunTest() {}
> 
> It seems like whatever caused the crash that's fixed by r110891 is now also fixed in some other way.
> 
> This is the example where clang currently marks A as invalid but keeps C valid:
> 
>   class A {};
>   class C : public A {};
>   A::RunTest() {}

Oh, yes, I see.  We should not be marking A as invalid here.

> I’m not sure what about the linked patch is making a base class invalid when a method in a derived class is invalidated, though?
> 
> No, the patch changes things to not mark a class invalid if one of its methods is invalid. This was the only place I could find where a class was marked invalid after its definition is complete. (Hence with this patch, C's superclass A will be valid too, only its method stays invalid, like with the first snippet).

I think that’s likely fine.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131220/51cf2117/attachment.html>