[PATCH] D54526: [AST] Pack BinaryOperator

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good - thanks! just a few things that could be cleaned up.



================
Comment at: include/clang/AST/Expr.h:3220
 
-  SourceLocation getExprLoc() const LLVM_READONLY { return OpLoc; }
-  SourceLocation getOperatorLoc() const { return OpLoc; }
-  void setOperatorLoc(SourceLocation L) { OpLoc = L; }
+  SourceLocation getExprLoc() const { return getOperatorLoc(); }
+  SourceLocation getOperatorLoc() const { return BinaryOperatorBits.OpLoc; }
----------------
Lost LLVM_READONLY somewhere along the way? Should it be added back in?


================
Comment at: include/clang/AST/Expr.h:3256-3259
+  static bool isPtrMemOp(Opcode Opc) {
+    return Opc == BO_PtrMemD || Opc == BO_PtrMemI;
+  }
+  bool isPtrMemOp() const { return isPtrMemOp(getOpcode()); }
----------------
I probably wouldn't add a new public member function here - I take it you added that to avoid calling getOpcode() twice? (either for performance or textual/source tersity?)

Calling it twice probably is OK for both performance and readability - but if you prefer otherwise, I guess you could do it with a temporary:

  bool isPetrMemOp() const {
    auto Opc = getOpcode();
    return Opc == ... ;
  }



================
Comment at: include/clang/AST/Expr.h:3369-3375
   bool isFPContractableWithinStatement() const {
-    return FPOptions(FPFeatures).allowFPContractWithinStatement();
+    return getFPFeatures().allowFPContractWithinStatement();
   }
 
   // Get the FENV_ACCESS status of this operator. Only meaningful for
   // operations on floating point types.
+  bool isFEnvAccessOn() const { return getFPFeatures().allowFEnvAccess(); }
----------------
Could do these two (& some of the other "refactor to use a common accessor, because the accessor is getting a bit more interesting") changes as NFC cleanup before this - but no big deal, just mention it in case in other/future changes that makes sense for you.


Repository:
  rC Clang

https://reviews.llvm.org/D54526