This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Added visibility context check for TypedefNameDecl.
ClosedPublic

Authored by balazske on Jul 10 2019, 6:00 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

balazske created this revision.Jul 10 2019, 6:00 AM
martong accepted this revision.Aug 30 2019, 5:52 AM
This revision is now accepted and ready to land.Aug 30 2019, 5:52 AM
shafik added inline comments.Aug 30 2019, 12:44 PM
lib/AST/ASTImporter.cpp
949 ↗(On Diff #208931)

I am not sure what case this covers? Can you elaborate? I see the case the condition above is catching.

unittests/AST/ASTImporterVisibilityTest.cpp
348 ↗(On Diff #208931)

Perhaps ExpectLink would be better as ExpectLinkedDeclChain and the same for ExpectNotLink.

It was actually confusing at first because link is an overload term.

352 ↗(On Diff #208931)

We need another set of tests for typedefs and using e.g. ExternTypedef and ExternUsing since they are equivalent.

It is worth noting that:

typedef int T;
typedef int T;

is not valid C99 see godbolt

balazske updated this revision to Diff 218310.Sep 2 2019, 1:54 AM

Improved the tests.

balazske updated this revision to Diff 218311.Sep 2 2019, 1:57 AM
  • Removed unneeded structs.
balazske marked 3 inline comments as done.Sep 2 2019, 2:09 AM

It is worth noting that:

typedef int T;
typedef int T;

is not valid C99 see godbolt

Should we handle this case? This can be special for C99 only when the declarations must be merged instead of linked. Probably this does not cause functional problems if we leave it as is.

lib/AST/ASTImporter.cpp
949 ↗(On Diff #208931)

This (last line) should mean that root level in source file (non-anonymous namespace) in different files is treated as same visibility context (like "extern" declarations). If namespace is different (anonymous and non-anonymous) the context is different.
For TypedefNameDecl there is no possibility of external formal linkage (it is like always external).

unittests/AST/ASTImporterVisibilityTest.cpp
348 ↗(On Diff #208931)

I can change it in a later patch.

shafik added a comment.Sep 3 2019, 2:29 PM

It is worth noting that:

typedef int T;
typedef int T;

is not valid C99 see godbolt

Should we handle this case? This can be special for C99 only when the declarations must be merged instead of linked. Probably this does not cause functional problems if we leave it as is.

I don't think it is critical to handle this case but I was surprised when I learned this, it is an edge case and it would be good to note this perhaps as a comment in the code.

shafik added inline comments.Sep 3 2019, 2:31 PM
clang/unittests/AST/ASTImporterVisibilityTest.cpp
343 ↗(On Diff #218311)

I don't have any objections to changing the names ExpectLink and ExpectNotLink in a subsequent patch but we should update then b/c they are confusing (because they are overloaded terms) and it takes some time to work backwards to understand the intent.

shafik accepted this revision.Sep 3 2019, 2:31 PM
balazske updated this revision to Diff 218610.Sep 4 2019, 1:47 AM
  • Added comment about repeated enums in C99.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2019, 7:12 AM