Page MenuHomePhabricator

[clang] Don't emit "no member" diagnostic if the lookup fails on an invalid record decl.
ClosedPublic

Authored by hokein on Aug 28 2020, 12:47 AM.

Details

Summary

The "no member" diagnostic is likely bogus.

Diff Detail

Event Timeline

hokein created this revision.Aug 28 2020, 12:47 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 28 2020, 12:47 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
hokein requested review of this revision.Aug 28 2020, 12:47 AM
sammccall accepted this revision.Sep 18 2020, 2:23 AM

This seems like fixing a fairly specific case of a general problem: if the base class is invalid I would expect to see similar problems in other places (e.g. unqualified lookup from inside the class).
But it fixes a diagnostic the libc++ folks think is important...

clang/lib/Sema/SemaExpr.cpp
2585

I found the wording of this example a bit hard to follow.

What about "e.g. if a class is invalid because it's derived from an invalid base class, then missing members were likely supposed to be inherited".

I'm actually not convinced this is merely an example - is there any other reason for invalidity other than invalid base class where this logic applies?

2587

nit: will fails -> will fail

2589

this is a fairly general check, but the reasoning really only applies to invalid bases.
If there's an easy way to check for that specifically instead, that'd be nice, but fine if not.

ldionne accepted this revision as: Restricted Project.Sep 18 2020, 12:40 PM
This revision is now accepted and ready to land.Sep 18 2020, 12:40 PM
hokein updated this revision to Diff 294635.Mon, Sep 28, 2:56 AM
hokein marked an inline comment as done.

Fix a comment.

clang/lib/Sema/SemaExpr.cpp
2585

I'm actually not convinced this is merely an example - is there any other reason for invalidity other than invalid base class where this logic applies?

I suppose there are other examples, but failed to find one.
Yeah, there are other reason that can cause an invalid RecordDecl (e.g. an invalid field), but it doesn't run into this code path. This code seems to be applied during template instantiation, I think it should be fine.

2589

we don't record that the base specifier is not attached due to the invalidity, we could introduce a bit to do that, not sure it is worth for this particular case, might be worth to reconsider if we see large regressions (currently, we don't have any regressions in existing tests).

This revision was landed with ongoing or failed builds.Mon, Sep 28, 6:10 AM
This revision was automatically updated to reflect the committed changes.