[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/Parse/Parser.h:1956-1957
                           AccessSpecifier AS, DeclSpecContext DSC);
-  void ParseEnumBody(SourceLocation StartLoc, Decl *TagDecl);
+  void ParseEnumBody(SourceLocation StartLoc, Decl *TagDecl,
+                     bool IgnorePrevDef = false);
   void ParseStructUnionBody(SourceLocation StartLoc, unsigned TagType,
----------------
Please remove this (now unused) parameter.


================
Comment at: lib/Parse/ParseDecl.cpp:4391
     if (TryConsumeToken(tok::equal, EqualLoc)) {
-      AssignedVal = ParseConstantExpression();
+      AssignedVal = ParseConstantExpression(NotTypeCast /*isTypeCast*/);
       if (AssignedVal.isInvalid())
----------------
Please revert this no-op change (left over from when you were passing in IgnorePrevDef).


================
Comment at: lib/Sema/SemaDecl.cpp:15423
+                         isa<EnumConstantDecl>(PrevDecl) &&
+                         PrevDecl->isFromASTFile() && !isVisible(PrevDecl);
     // When in C++, we may get a TagDecl with the same name; in this case the
----------------
The `isFromASTFile` check here is not correct: whether a declaration is from an AST file is independent of whether it might be not visible due to being in a module (local submodule visibility rules allow a local declaration to not be visible). The `Modules` check is also not correct: we do visibility checking for local `#include`s of module headers under `-fmodules-local-submodule-visibility -fno-modules`.

We also shouldn't diagnose ambiguity if we find two different hidden previous declarations, but we will do so (within `LookupSingleName`) with this approach.

Digging into this a bit more, I think the root of the problem is that the `ND->isExternallyVisible()` call in `LookupResult::isHiddenDeclarationVisible` is wrong. Redeclaration lookup should never find hidden enumerators in C, because they do not have linkage (C11 6.2.2/6). (The same is true in C++, but I don't know whether we can apply the same thing there too, due to the different merging rules.) A function `foo` in some source file should not conflict with an enumerator `foo` in a non-imported module, for instance.


https://reviews.llvm.org/D31778