ChuanqiXu added inline comments.
================
Comment at: clang/test/Modules/inconsist-export-template.cpp:19-23
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1<int>() {
+
----------------
dblaikie wrote:
> urnathan wrote:
> > ChuanqiXu wrote:
> > > iains wrote:
> > > > iains wrote:
> > > > > ChuanqiXu wrote:
> > > > > > urnathan wrote:
> > > > > > > I don't think we should be testing for ill-formed code here. We
> > > > > > > want to verify that explicit instantiations, explicit
> > > > > > > specializations and implicit instantiations all get the expected
> > > > > > > linkage -- both external linkage on exported entities, module
> > > > > > > linkage on non-exported module-purview entities.
> > > > > > I think it could add an `expected-error` once we complete the check
> > > > > > in compiler so I added the FIXME here.
> > > > > would it be possible to find a suitable place in the source for the
> > > > > FIXME?
> > > > > I would be concerned that it could get forgotten in the test-case.
> > > > or maybe just file a PR for accepts invalid code?
> > > I would like to send an issue after the patch to remind me not forgetting
> > > it. The issue is needed after the patch since it would crash too. I
> > > prefer to remain the FIXME here so that it would look like a pre-commit
> > > test case, which should be good.
> > The way to not forget about this is to file a bug and assign it to
> > yourself. Not add a known-wrong testcase.
> FWIW, I do sometimes put in known-bad test cases to document existing issues
> and also to make it easier to find existing test coverage/a good home for the
> test case (often I/we end up adding a new test file, because it's not obvious
> where related existing test coverage is).
>
> If this is the right place for the future test case, and it documents a
> limitation with the existing behavior, I wouldn't be totally against
> including it here, with a FIXME and reference to a filed bug, ideally (though
> I think I've done this without a bug filed too).
Agree with @dblaikie here, I filed an issue
(https://github.com/llvm/llvm-project/issues/54189) assigned to myself and a
reference it. @urnathan , a key reason why I don't move the known-wrong test
case is that this case would crash now. And I think it is good to test it
wouldn't crash. Crash shouldn't be good all the time.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120397/new/
https://reviews.llvm.org/D120397
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
