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

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 5 inline comments as done.
mibintc added a comment.

A couple replies to @erichkeane



================
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; }
----------------
erichkeane wrote:
> 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.
It's only used during serialization (ASTReader); I guess the node has already been allocated by then so it's superfluous, because the allocation point could set this field. 


================
Comment at: clang/include/clang/AST/Expr.h:2268
+  FPOptions getFPFeatures(const LangOptions &LO) const {
+    if (UnaryOperatorBits.HasFPFeatures)
+      return getStoredFPFeatures();
----------------
erichkeane wrote:
> 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.
John suggested the name getStored hasStored as "less tempting" names. The getStored and hasStored are only used during Serialization.  John suggested the getFPFeatures function as the public interface and it uses the LangOptions parameter.  The features are saved in the node if they can't be recreated from the command line floating point options (due to presence of floating point pragma)


================
Comment at: clang/include/clang/AST/Expr.h:2701
 
+  FPOptions FPFeatures;
+
----------------
erichkeane wrote:
> 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.
whoops i meant that to go to the CallExprBits


================
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);
 
----------------
erichkeane wrote:
> Is this an unrelated change?  What is the purpose for this?
it's a NFC the llvm:: prefix wasn't needed. maybe the clang formatter did that? 


================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:684
 void ASTStmtReader::VisitUnaryOperator(UnaryOperator *E) {
+  bool hasFP_Features;
   VisitExpr(E);
----------------
erichkeane wrote:
> Rather than this variable, why not just ask 'E' below?
yes i could do that. it would be a function call


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72841