Page MenuHomePhabricator

[ASTImporter] Added visibility check for scoped enums.
ClosedPublic

Authored by balazske on Feb 13 2020, 6:49 AM.

Details

Summary

ASTImporter makes now difference between C++11 scoped enums with same
name in different translation units if these are not visible outside.
Enum declarations are linked into decl chain correctly.

Diff Detail

Event Timeline

balazske created this revision.Feb 13 2020, 6:49 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
shafik added inline comments.Feb 13 2020, 9:58 AM
clang/lib/AST/ASTImporter.cpp
2600

Can you explain why we need to check D->isThisDeclarationADefinition()

Does the test added hit all the combination of cases?

martong added inline comments.Feb 13 2020, 12:22 PM
clang/lib/AST/ASTImporter.cpp
2600

By the time of this check, we already know that D and the existing (found) decl are structurally equivalent. If they are both definitions then they must be the same, so we keep on with the existing. Actually, this is not unorthodox with enums, this is the pattern that we follow with all kind of declarations.

shafik accepted this revision.Feb 13 2020, 2:56 PM

LGTM

This revision is now accepted and ready to land.Feb 13 2020, 2:56 PM
a_sidorin accepted this revision.Feb 13 2020, 3:40 PM
a_sidorin added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
4870 ↗(On Diff #244422)

Maybe it's better to use just non-raw string literals here?

Decl *ToTU = getToTuDecl("enum class E;");
balazske updated this revision to Diff 244582.Feb 14 2020, 1:09 AM

Added to the generic tests (and removed one redundant test).

balazske marked an inline comment as done.Feb 14 2020, 1:16 AM
balazske added inline comments.
clang/lib/AST/ASTImporter.cpp
2600

Good observation: The enum class case could be added to the generic redecl and ODR tests. This makes the single test in ASTImporterTest.cpp unnecessary, this case should be covered by the tests in ASTImporterGenericRedeclTest.cpp.

This revision was automatically updated to reflect the committed changes.