[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.
----------------
rsmith wrote:
> Typo 'abd'
Typo 'compatibale' =)


================
Comment at: lib/Parse/ParseDecl.cpp:4307
+    Decl *D = SkipBody.CheckSameAsPrevious ? SkipBody.New : TagDecl;
+    //ParseEnumBody(StartLoc, D);
+    ParseEnumBody(StartLoc, D,
----------------
Commented-out code, please remove.


================
Comment at: lib/Parse/ParseDecl.cpp:4313-4321
+    if (SkipBody.CheckSameAsPrevious) {
+      if (!Actions.hasStructuralCompatLayout(TagDecl, SkipBody.New)) {
+        // Bail out.
+        DS.SetTypeSpecError();
+        return;
+      }
+      // Make the previous decl visible.
----------------
The parser shouldn't be doing this itself, please move this to a function on `Sema` (`ActOnDuplicateDefinition` or something) and call that from here.


================
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();
----------------
bruno wrote:
> rsmith wrote:
> > 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?
> Thanks for pointing it out, I totally missed this approach. Your suggestion works and I'll change the patch accordingly. However, `IgnorePrevDef` still needs to be threaded in `ParseEnumBody` and `ActOnEnumConstant` in order to prevent the latter to emit `err_redefinition_of_enumerator`-like diagnostics.
I think the problem is that we don't take module visibility into account when doing redefinition checking for enumerators. Instead of passing through this flag, we should probably just ignore hidden declarations when checking for a redefinition of an enumerator.


https://reviews.llvm.org/D31778