Page MenuHomePhabricator

[Sema] Fix friend destructor declarations after D130936
ClosedPublic

Authored by royjacobson on Aug 9 2022, 10:48 PM.

Details

Summary

I accidentally broke friend destructor declarations in D130936.

Modify it to skip performing the destructor name check if we have a dependent friend declaration.

Diff Detail

Event Timeline

royjacobson created this revision.Aug 9 2022, 10:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 10:48 PM
royjacobson requested review of this revision.Aug 9 2022, 10:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 10:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
royjacobson planned changes to this revision.Aug 9 2022, 10:50 PM

Don't regress on invalid (non-dependent) friend destructor declarations.

Hi @hubert.reinterpretcast, thanks for the quick catch!

I have posted this initial patch, but it still misses the case

template <typename T>
struct E {
  friend T::S::~V();
};

Something goes wrong there that I haven't figured out yet. Do you think I should land this as-is to fix the master?

clang/lib/Sema/SemaDecl.cpp
11518–11520

There's nothing we can diagnose about this without an instantiation (because S could be an alias for a class having V as the injected-class-name).

It is, however, true that we don't diagnose this even with problematic instantiations:

struct R { struct V; ~R(); };
struct QQ { using S = R; };

template <class T>
struct A { friend T::S::~V(); };

A<QQ> a;
clang/lib/Sema/SemaDecl.cpp
11522

This condition appears to be true even when the friend itself is not dependent. The error message changes for:

struct C;
struct B { ~B(); };
template <typename T>
struct A {
  friend B::~C();
};

(causing the template and non-template cases to generate different messages).

clang/test/SemaCXX/member-class-11.cpp
36–40

Please replace this with the case where there is an instantiation. Also, the prior change to the release notes in https://reviews.llvm.org/D130936 should be adjusted to reflect the new scope of what is fixed.

Update to handle some edge cases better.

Add another test case.

Updated the patch and added more test coverage.

I still don't diagnose the dependent friend case, but I didn't see how to do it easily.

clang/lib/Sema/SemaDecl.cpp
11518–11520

Wow, thanks! That's some edge case I haven't thought about..

clang/test/SemaCXX/member-class-11.cpp
36–40

Updated the test cases accordingly.

I don't think there's things to add in the release notes as this is just fixing breakage from my previous patch, not really diagnosing new cases?

royjacobson edited the summary of this revision. (Show Details)Aug 13 2022, 12:47 PM

This LGTM; thanks!

I still don't diagnose the dependent friend case, but I didn't see how to do it easily.

Maybe adding code to TemplateDeclInstantiator::VisitFriendDecl will do the trick.

clang/test/SemaCXX/member-class-11.cpp
36–40

Okay; I guess "invalid destructor names were incorrectly accepted on template classes" uses "on" and not "in" (and can be read to match the current scope of the fix).

This revision is now accepted and ready to land.Aug 15 2022, 9:28 PM