[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/DiagnosticCommonKinds.td:127
 
+def warn_eval_method_setting_is_meaningless_in_value_unsafe_context: Warning<
+  "setting the eval method via the `ffp-eval-method` option or the `pragma clang fp eval_method`"
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:128-129
+def warn_eval_method_setting_is_meaningless_in_value_unsafe_context: Warning<
+  "setting the eval method via the `ffp-eval-method` option or the `pragma clang fp eval_method`"
+  " is meaningless when numeric results of floating-point calculations aren't value-safe.">, InGroup<Pragmas>;
 }
----------------
FWIW, I'm still not quite certain what the diagnostic is telling me. I don't know what "value-safe" means in this context, so we may need to tweak this further.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1065
 
+  if (Tok.isNot(tok::annot_pragma_fp) &&
+      getLangOpts().getFPEvalMethod() != LangOptions::FEM_UnsetOnCommandLine &&
----------------
This looks like it's going to diagnose on every compound statement regardless of whether the function contains any floating-point operations at all. If so, that's way too chatty.

It seems like the issue here is "the command line options disagree", so why is this not a frontend warning when processing the command line options? 


================
Comment at: clang/test/Sema/eval-method-with-unsafe-math.c:37
+float f4(float a, float b, float c) {
+  // expected-warning at +1{{setting the eval method via the `ffp-eval-method` option or the `pragma clang fp eval_method` is meaningless when numeric results of floating-point calculations aren't value-safe.}}
+#pragma clang fp eval_method(double)
----------------
Once we issue the warning for the pragma, we should not issue the warning a second time for the statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122155