aaron.ballman added a comment. In D147888#4467002 <https://reviews.llvm.org/D147888#4467002>, @Krishna-13-cyber wrote:
> In D147888#4466998 <https://reviews.llvm.org/D147888#4466998>, @aaron.ballman > wrote: > >> I think the existing wording is pretty reasonable, changing "non-static >> declaration" into "extern declaration" isn't really giving the user any more >> information to help them resolve the issue. "please pick exactly one" >> doesn't seem likely to help the user either -- they already know there's a >> conflict, so picking one is already the solution the user is likely to have >> in mind. The hard part for the user is with figuring which one to pick, but >> we have no way to help them make that choice. So I'm not certain changes are >> needed in this space (I'm not opposed either), but I do think the idea @cjdb >> had to combine all these diagnostics into one using `%select` could be >> helpful. However, there are enough options that it could also be pretty >> complex to reason about. There's static, extern, and thread-local, as well >> as "non-" versions of each of those, so there are quite a few combinations. > > Yes I agree with this, > Changing the whole set of diagnostics will be quite challenging, but I think > `extern` has an edge over `non-static` declaration when we are able to see > the diagnostics in gcc if are not going with the `pick-exactly one` > convention. I can sort of buy the argument that `extern` has a slight edge, but I think what would really help are test cases for all the various combinations so we can see them at once (static, extern. no linkage, thread local vs each counterpart). I'm concerned that the code you added conflicts with the existing code immediately below your changes and the standards wording quoted above your changes, but I left a comment on what would make me more comfortable. ================ Comment at: clang/lib/Sema/SemaDecl.cpp:4641-4657 + if (New->hasExternalStorage() && + Old->getCanonicalDecl()->getStorageClass() == SC_Static && + Old->isLocalVarDeclOrParm()) { + Diag(New->getLocation(), diag::err_extern_static) << New->getDeclName(); + Diag(OldLocation, PrevDiag); + return New->setInvalidDecl(); + } ---------------- ================ Comment at: clang/test/SemaCXX/extern_static.cpp:7 +} \ No newline at end of file ---------------- Be sure to add the newline back. Also, I think there should be a C run line for the test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147888/new/ https://reviews.llvm.org/D147888 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
