junaire added a comment.
In D119405#3310695 <https://reviews.llvm.org/D119405#3310695>, @aaron.ballman
wrote:
> Thank you for the patch!
>
> I don't think this is an improvement; the diagnostic text goes from being
> correct in C but odd in C++ to being correct in C++ but odd in C (the two
> standards use different terminology). We're already inconsistent with which
> we prefer (we have diagnostics that use `literal` and others that use
> `constant`), and perhaps it'd make sense to start using
> `%select{constant|literal}N` in the individual diagnostics to switch the
> wording based on the current language mode. That said, I don't know that
> there's a ton of value in that change (it seems unlikely that anyone is
> actually confused what the diagnostic is saying today).
Thanks for spending your valuable time reviewing this! I think your words make
sense. And I wonder if the patch is rejected, what should I do? I mean does
phabricator have something like close PR in github?
> Also, this changes the text of a diagnostic but doesn't update any tests, so
> precommit CI is failing. :-)
Well, I already run `ninja check-clang` locally to make sure it doesn't break
anything, and I didn't find any tests are falling in the CI, so I guess maybe
the CI is just not stable enough?
> Before changing things in this review, I think we should go back to the bug
> report to find out why the request was made and what issues there are with
> the diagnostic. If it's a matter of "wrong terminology in a language mode",
> I'm of the opinion that's not really an issue in this particular case (unless
> there's other confusion that's caused by the terminology). I'll start the
> discussion there.
Good point! thanks for telling me about how the community works, I'm still
learning :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119405/new/
https://reviews.llvm.org/D119405
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
