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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to