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