[PATCH] D53847: [C++2a] P0634r3: Down with typename!

[Alan Zhao via Phabricator via cfe-commits]

ayzhao marked 2 inline comments as done.
ayzhao added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:367
+        // with this method returning a non-null ParsedType?
+        if (isa<ExportDecl>(CurContext))
+          return nullptr;
----------------
ayzhao wrote:
> erichkeane wrote:
> > Hmm... this scares me quite a bit, I don't really know what other fallout results from this, or other code that would hit here.
> > 
> > We should probably spend more time making sure we understand this better.
> Following up with my [previous comment](https://reviews.llvm.org/D53847?id=459205#inline-1291007) about the failing test that this hack was supposed to fix:
> 
> I added the `typename` keyword to the `export template` statement and lo and behold, Clang emits the same "declaration does not declare anything" diagnostic instead of the namespace scope diagnostic [0].
> 
> To me, this feels like that the [original test case was wrong](https://github.com/llvm/llvm-project/blob/a707675dbba9ca3ec6e668f86fea2240a85ca171/clang/test/CXX/module/module.interface/p2-2.cpp#L18):
> 
> 1. There should've been a `typename` keyword in the `export` statement. Perhaps the author of the test wasn't aware that the `typename` keyword was required, or perhaps the author already assumed that P0634r3 was already implemented in clang.
> 1. The test currently passing (with Clang emitting the namespace diagnostic instead of the declaration diagnostic) seems to be a freak coincidence.
> 
> I'm not familiar at all with C++ modules - is this a bug with exporting types?
> 
> [0]: https://godbolt.org/z/PYvh68Tq7
small fix: I linked to the wrong line when referencing the original test case. The link should be https://github.com/llvm/llvm-project/blob/a707675dbba9ca3ec6e668f86fea2240a85ca171/clang/test/CXX/module/module.interface/p2-2.cpp#L17


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53847