[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/Sema/Sema.h:1464
 
+  /// Determine if \p D abd \p Suggested have a structurally compatibale
+  /// layout as described in C11 6.2.7/1.
----------------
Typo 'abd'


================
Comment at: lib/Parse/ParseDecl.cpp:4236-4237
 
   Sema::SkipBodyInfo SkipBody;
+  Sema::CheckCompatTagInfo CheckCompatTag;
   if (!Name && TUK == Sema::TUK_Definition && Tok.is(tok::l_brace) &&
----------------
Do we really need both of these? The new stuff seems to be a natural extension of `SkipBodyInfo`, to say "parse it, check it's the same as the previous definition, then throw the new one away".


================
Comment at: lib/Sema/SemaDecl.cpp:12929
+  /// here is passed back to the parser, allowing the tag body to be parsed.
+  auto createTagFromNewDecl = [&](void) -> TagDecl * {
+    assert(!getLangOpts().CPlusPlus && "not meant for C++ usage");
----------------
No `void` here, please.


================
Comment at: lib/Sema/SemaDecl.cpp:12962
+      // It is important for implementing the correct semantics that this
+      // happen here (in act on tag decl). The #pragma pack stack is
+      // maintained as a result of parser callbacks which can occur at
----------------
"act on tag decl" -> ActOnTag?


================
Comment at: lib/Sema/SemaDecl.cpp:13383-13384
+              // Allow ODR for ObjC/C in the sense that we won't keep more that
+              // one definition around (merge them). However, ensure the decl
+              // pass the structural compatibility check in C11 6.2.7/1.
+              bool AllowODR = getLangOpts().CPlusPlus || getLangOpts().ObjC1 ||
----------------
Plurality mismatch in "decl pass"


================
Comment at: lib/Sema/SemaExpr.cpp:2198-2209
+  if (R.isAmbiguous()) {
+    if (!IgnorePrevDef)
+      return ExprError();
+    // We already know that there's a hidden decl included in the lookup,
+    // ignore it and only use the first found (the local) one...
+    auto RF = R.makeFilter();
+    NamedDecl *ND = R.getRepresentativeDecl();
----------------
This is gross. In order to make this work, you'd need to propagate the `IgnorePrevDef` flag through the entire expression parsing machinery.

Instead of this, how about deferring making the old definition visible until after you complete parsing the new one and do the structural comparison?


https://reviews.llvm.org/D31778