[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:4394
+        isa<EnumDecl>(D))
+      continue;
+
----------------
rsmith wrote:
> rjmccall wrote:
> > This is essentially part of the next paragraph.
> > 
> > I believe `friend` declarations also do not declare "members", under the same logic allowing `static_assert`.
> I think you're right, but that's a defect in the wording -- a friend definition, at least, could use the class in a way that requires a linkage calculation, and the design intent as approved by the Evolution Working Group did not permit friend declarations. I'll take this back to the committee for some rewording, but I think it makes sense to proceed under the assumption that friends won't be allowed. In any case, I'm updating the diagnostic to treat friends as a special case, since they're not member declarations.
Could it?  I think the same inability to write a self-reference that makes the C-like cases okay should also apply to friends.  If you can write a self-reference in a friend declaration, you can write one in a template argument list in the type of a field declaration.  Well, okay,  I suppose a friend function *definition* might need to be forbidden.

Anyway, I don't think it's actually a problem if the committee wants to outlaw friends here, just wondering at the effective rule they're aiming for in case we need to figure out how to apply it to extensions.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:4404
+    if (MemberRD->isLambda())
+      return {NonCLikeKind::Lambda, MemberRD->getSourceRange()};
+
----------------
rsmith wrote:
> rjmccall wrote:
> > Do lambdas in nested expressions really get added to the class's decls list?  I wouldn't have expected that, but it definitely makes this check a lot easier.
> They end up in some enclosing `DeclContext`, which is either the class itself or something else that we also disallow.
Yeah, obviously their DC should be the class, but I didn't think the decl-to-DC link always implied membership in the decls list.  But hey, it seems to work in the case we care about.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:4473
 
-  // Otherwise, set this is the anon-decl typedef for the tag.
+  // If we compute the linkage of a C-compatible type early, that's a bug.
+  assert(!TagFromDeclSpec->hasLinkageBeenComputed() &&
----------------
Might be good to tell future maintainers how to fix it: probably, by adding something to the forbidden list above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74103/new/

https://reviews.llvm.org/D74103