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
