[PATCH] D122155: Add warning when eval-method is set in the presence of value unsafe floating-point calculations.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:50-53
+def warn_eval_method_setting_via_option_in_value_unsafe_context : Warning<
+    "setting the eval method via '-ffp-eval-method' has not effect when numeric "
+    "results of floating-point calculations aren't value-safe.">,
+    InGroup<IncompatibleFPOpts>;
----------------
Unless you have a strong reason for this to be a warning, this seems like a situation we should diagnose as an error with a much clearer message.


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:127
 def UnsupportedFPOpt : DiagGroup<"unsupported-floating-point-opt">;
+def IncompatibleFPOpts : DiagGroup<"incompatible-floating-point-opts">;
 def UnsupportedCB : DiagGroup<"unsupported-cb">;
----------------
This change won't be needed any longer.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6479-6481
+  "setting the eval method via the `pragma clang fp eval_method` "
+  "has no effect when numeric results of floating-point calculations aren't "
+  "value-safe.">,
----------------
Similar wording suggestion here as above -- this makes it more clear what the issue is and how the user can fix it. You'll have to figure out which pragmas and which command line options set the language options you're checking for the predicate. But the goal is to let users know where the conflict is.

Bonus points if you can add a note to point to the previous pragma that caused the conflict with `clang fp eval_method` so the user doesn't have to hunt for it. However, I don't insist on that as it may be difficult to track. But if it isn't difficult, it would allow us to improve the `%select` slightly to something like:   `"'#pragma clang fp eval_method' cannot be used with %select{'#pragma clang fp reassociate'|'-mreassociate'|'#pragma whatever'|'-fwhatever'|..}0">,`



================
Comment at: clang/lib/Sema/SemaAttr.cpp:492
+    Diag(Loc,
+         diag::warn_eval_method_setting_via_pragma_in_value_unsafe_context);
   FpPragmaStack.Act(Loc, PSK_Set, StringRef(), NewFPFeatures);
----------------
zahiraam wrote:
> Not sure if I should repeat the comment here (same one than in CompilerInvocation.cpp?).
Er, given that they're pretty involved comments, I'd only put the comments in one place, and then have the other one say "See the comments in <some reasonable identifying location> for more information about why this is diagnosed." or something to that effect.


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

https://reviews.llvm.org/D122155