[PATCH] D119405: [Clang][Sema] Use C++ standard terminology in clang diagnostics.

[Jun Zhang via Phabricator via cfe-commits]

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