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