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

[Alan Zhao via Phabricator via cfe-commits]

ayzhao marked an inline comment 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:
> 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
So my belief now is that the test `p2-2.cpp` is broken. I've removed the `FIXME` hack, and I created D134578 to fix the test. To keep the bots happy, I also cherry-picked D134578 into this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53847