[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

@rjmccall  : can you comment on the CallExpr's trailing storage issue?  And give advice as to what you meant in the last review?



================
Comment at: clang/include/clang/AST/Expr.h:2122
+    return *getTrailingObjects<FPOptions>();
+  }
+  const FPOptions &getTrailingFPFeatures() const {
----------------
needs a newline between functions.


================
Comment at: clang/include/clang/AST/Expr.h:2251
+  /// allocated in Trailing Storage
+  void setHasStoredFPFeatures(bool B) { UnaryOperatorBits.HasFPFeatures = B; }
+  bool hasStoredFPFeatures() const { return UnaryOperatorBits.HasFPFeatures; }
----------------
Is this really useful/usable at all?  It seems to me that since this would require re-allocating this object that noone should be able to set this after construction.


================
Comment at: clang/include/clang/AST/Expr.h:2255
+  /// Get FPFeatures from trailing storage
+  FPOptions getStoredFPFeatures() const {
+    assert(hasStoredFPFeatures());
----------------
Why is this a separate function from getTrailingFPFeatures?  


================
Comment at: clang/include/clang/AST/Expr.h:2256
+  FPOptions getStoredFPFeatures() const {
+    assert(hasStoredFPFeatures());
+    return getTrailingFPFeatures();
----------------
If they have to be separate, the assert here doesn't really make sense, since getTrailingFPFeatures has the same assert.


================
Comment at: clang/include/clang/AST/Expr.h:2261
+  void setStoredFPFeatures(FPOptions F) {
+    assert(UnaryOperatorBits.HasFPFeatures);
+    getTrailingFPFeatures() = F;
----------------
For the asserts, we should probably prefer using the hasStoredFPFeatures function.


================
Comment at: clang/include/clang/AST/Expr.h:2268
+  FPOptions getFPFeatures(const LangOptions &LO) const {
+    if (UnaryOperatorBits.HasFPFeatures)
+      return getStoredFPFeatures();
----------------
Is there use in having both this AND the get-stored, as opposed to just making everyone access via the same function?  At least having 2 public versions aren't very clear what the difference is to me.


================
Comment at: clang/include/clang/AST/Expr.h:2701
 
+  FPOptions FPFeatures;
+
----------------
This type already has trailing-storage type stuff.  I think in the past review @rjmccall encouraged you to put this in the fake-trailing-storage  like above.


================
Comment at: clang/include/clang/Basic/LangOptions.h:197
   static constexpr unsigned FPR_ToNearest =
-      static_cast<unsigned>(llvm::RoundingMode::NearestTiesToEven);
+      static_cast<unsigned>(RoundingMode::NearestTiesToEven);
 
----------------
Is this an unrelated change?  What is the purpose for this?


================
Comment at: clang/include/clang/Basic/LangOptions.h:389
   // Used for serializing.
   explicit FPOptions(unsigned I)
+      : fp_contract(I & 3), fenv_access((I >> 2) & 1), rounding((I >> 3) & 7),
----------------
Is this the same logic as getFromOpaqueInt?  If so, we should probably just call that.


================
Comment at: clang/include/clang/Sema/Sema.h:558
 
+  // This stacks the current state of Sema.CurFPFeatures
+  PragmaStack<unsigned> FpPragmaStack;
----------------
This comment is really oddly phrased and uses the 'stack'-noun as a verb?  Something like: (please feel free to wordsmith): "This stack tracks the current state of Sema.CurFPFeatures."?


================
Comment at: clang/lib/AST/Expr.cpp:1309
   unsigned NumPreArgs = PreArgs.size();
+  FPFeatures = FP_Features;
   CallExprBits.NumPreArgs = NumPreArgs;
----------------
Is there a reason this isn't a member initializer?


================
Comment at: clang/lib/Parse/ParsePragma.cpp:667
+  uintptr_t Value = reinterpret_cast<uintptr_t>(Tok.getAnnotationValue());
+  Sema::PragmaMsStackAction Action =
+      static_cast<Sema::PragmaMsStackAction>((Value >> 16) & 0xFFFF);
----------------
Oh boy these are some magic lookin numbers... Can you document these two lines?


================
Comment at: clang/lib/Parse/ParsePragma.cpp:2534
+  PP.Lex(Tok);
+  if (Tok.isNot(tok::l_paren)) {
+    PP.Diag(FloatControlLoc, diag::err_expected) << tok::l_paren;
----------------
Replace this with BalancedDelimiterTracker instead, it gives consistent errors and are a bit easier to use.  Additionally, I think it does some fixups that allow us to recover better.

I'd also suggest some refactoring with the PragmaFloatControlKind if/elses below.  Perhaps handle the closing paren at the end, and do a switch-case for that handling.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:428
+  case PFC_NoExcept:
+    switch (Value) {
+    default:
----------------
I guess I don't get why you're switching on both here?  Can the two just be combined?  I don't know if the 'NewValue = CurFPFeatures.getAsOpaqueInt();
    FpPragmaStack.Act(Loc, Action, StringRef(), NewValue);'

part is sufficiently motivated to do 2 separate switches.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1023
+      Diag(Loc, diag::err_pragma_fenv_requires_precise);
     CurFPFeatures.setAllowFEnvAccess();
     break;
----------------
Should we still be setting this even if there was an error?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:382
 void Sema::ActOnStartOfCompoundStmt(bool IsStmtExpr) {
+
   PushCompoundScope(IsStmtExpr);
----------------
unrelated change?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:399
+  if (getCurFPFeatures().isFPConstrained()) {
+    // Mark the current function as usng floating point constrained intrinsics
+    if (FunctionDecl *F = dyn_cast<FunctionDecl>(CurContext)) {
----------------
Don't use curleys for single liners, both of these probably shouldn't need curleys at all.  Comment could be at the top for clarity.


================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:684
 void ASTStmtReader::VisitUnaryOperator(UnaryOperator *E) {
+  bool hasFP_Features;
   VisitExpr(E);
----------------
Rather than this variable, why not just ask 'E' below?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72841