[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

These changes will also need a release note at some point.



================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:662-665
+def err_type_definition_cannot_be_modified : Error<
+  "%0 type definition cannot be modified inside a scope containing "
+  "'#pragma clang fp eval_method(%1)' when a command line "
+  "option 'ffp-eval-method=%2' is used">;
----------------
I think we should add some documentation as to why this error happens. Naively, users expect the pragma to override the command line (the command line is the "default" and the pragma overrides that in the cases the default is wrong), so I'd imagine a user getting this error would ask, "But why?"


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1122-1123
+    return "source";
+  default:
+    llvm_unreachable("unexpected eval method value");
+  }
----------------
Does something assert that we never try to call this with `FEM_Indeterminable`?


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1198-1210
+    if (Tok.is(tok::identifier)) {
+      if (Tok.getIdentifierInfo()->getName().str() == "float_t" ||
+          Tok.getIdentifierInfo()->getName().str() == "double_t") {
+        if (getLangOpts().getFPEvalMethod() !=
+                LangOptions::FPEvalMethodKind::FEM_UnsetOnCommandLine &&
+            PP.getLastFPEvalPragmaLocation().isValid() &&
+            PP.getCurrentFPEvalMethod() != getLangOpts().getFPEvalMethod())
----------------
This is not correct -- it's only going to catch the case where the user literally writes `float_t` as the first token in a statement, so it's going to miss `const float_t t` and `typedef float_t frobble; frobble t;` etc. Also, this seems like it will impact compile time overhead because that's going to be performing a lot of useless string comparisons.

This cannot be handled at the level of the parser, it needs to be handled within Sema. I think you'll want it to live somewhere around `GetFullTypeForDeclarator()` in SemaType.cpp.

There are other situations that should be tested:
```
char buffer[sizeof(float_t)]; // This seems like something we'd want to prevent?
typedef float_t foo; // This seems like something that's harmless to accept?
using quux = float_t; // Same here as above
foo bar; // This, however, is a problem

extern float_t blah(); // This also seems bad

_Generic(1, float_t : 0); // Maybe a bad thing?

auto lam = [](float_t f) { return f; }; // Bad?

float f = (float_t)12; // What about this? Note, the float_t is only used in the cast...
```



================
Comment at: clang/test/CodeGen/abi-check-1.c:1-2
+// RUN: %clang_cc1  -isystem %S/Inputs -triple x86_64-linux-gnu \
+// RUN: -emit-llvm -o - %s | FileCheck %s
+
----------------
Why are these codegen tests when the diagnostic (should be) in Sema?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146148