[PATCH] D64480: [ASTImporter] Added visibility context check for TypedefNameDecl.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 3 inline comments as done.
balazske added a comment.

In D64480#1653629 <https://reviews.llvm.org/D64480#1653629>, @shafik wrote:

> It is worth noting that:
>
>   typedef int T;
>   typedef int T;
>
>
> is not valid C99 see godbolt <https://godbolt.org/z/638lXv>


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.



================
Comment at: lib/AST/ASTImporter.cpp:949
+    return Importer.GetFromTU(Found) == From->getTranslationUnitDecl();
+  return From->isInAnonymousNamespace() == Found->isInAnonymousNamespace();
+}
----------------
shafik wrote:
> I am not sure what case this covers? Can you elaborate? I see the case the condition above is catching.
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).


================
Comment at: unittests/AST/ASTImporterVisibilityTest.cpp:348
+        ::testing::Values(
+            std::make_tuple(ExternTypedef, ExternTypedef, ExpectLink),
+            std::make_tuple(ExternTypedef, AnonTypedef, ExpectNotLink),
----------------
shafik wrote:
> Perhaps `ExpectLink` would be better as `ExpectLinkedDeclChain` and the same for `ExpectNotLink`.
> 
> It was actually confusing at first because link is an overload term.
I can change it in a later patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64480/new/

https://reviews.llvm.org/D64480