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

Thu Feb 10 04:59:30 PST 2022

aaron.ballman added a comment.

In D119405#3310711 <https://reviews.llvm.org/D119405#3310711>, @junaire wrote:

> 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?

I'm very happy to help! I really appreciate the patch; I see it was marked as a good first issue patch, so I'm sorry for the back-and-forth.

If we decide not to go forward, Phabricator has an "Abandon Revision" option under the "Add Action" drop-down menu. I'd say let's leave the patch open until the discussion on GitHub concludes, just in case.

>> 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?

Oh no! I see now that the precommit CI pipeline never ran the Clang tests because the build failed for unrelated reasons. So yeah, our CI wasn't very stable. I took a deeper look and we are lacking any test coverage for these diagnostic in the test suite, which is unfortunate. If you are looking for a good first issue, that might actually be a great one to tackle -- you could add a new test case in `clang/test/Sema/` named `float-constants.c` (or something along those lines) to add new test cases for both of those diagnostics. Then we'd have test coverage for the diagnostic to help us catch regressions in behavior. (You get bonus points for covering the different kinds of floating-point literals, like hexadecimal floats, and testing edge cases vs extreme cases vs normal cases, etc.)

>> 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 :-)

Welcome to the community -- there's a lot to learn, but we're happy to help you learn it!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119405/new/

https://reviews.llvm.org/D119405