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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits