[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC
Richard Smith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri May 19 11:48:18 PDT 2017
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
More information about the cfe-commits
mailing list