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

[junaire]

Summary

Clang diagnostics should say "floating-point literal" instead of "floating-point constant"
according to C++ standard.

Close #53100

[aaron.ballman]

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

Also, this changes the text of a diagnostic but doesn't update any tests, so precommit CI is failing. :-)

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.

[junaire]

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

[aaron.ballman]

In D119405#3310711, @junaire wrote:

In 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!

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_ice_too_large : Error<
   "represented in a %1-bit %select{signed|unsigned}2 integer type">;
 def err_expr_not_string_literal : Error<"expression is not a string literal">;
 
 // Semantic analysis of constant literals.
 def ext_predef_outside_function : Warning<
   "predefined identifier is only valid inside function">,
   InGroup<DiagGroup<"predefined-identifier-outside-function">>;
 def warn_float_overflow : Warning<
-  "magnitude of floating-point constant too large for type %0; maximum is %1">,
+  "magnitude of floating-point literal too large for type %0; maximum is %1">,
    InGroup<LiteralRange>;
 def warn_float_underflow : Warning<
-  "magnitude of floating-point constant too small for type %0; minimum is %1">,
+  "magnitude of floating-point literal too small for type %0; minimum is %1">,
   InGroup<LiteralRange>;
 def warn_double_const_requires_fp64 : Warning<
   "double precision constant requires %select{cl_khr_fp64|cl_khr_fp64 and __opencl_c_fp64}0, "
   "casting to single precision">;
 def err_half_const_requires_fp16 : Error<
   "half precision constant requires cl_khr_fp16">;
 
 // C99 variable-length arrays