ASTImporter makes now difference between typedefs and type aliases
with same name in different translation units
if these are not visible outside.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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. |
unittests/AST/ASTImporterVisibilityTest.cpp | ||
348 ↗ | (On Diff #208931) | I can change it in a later patch. |
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.
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. |