[PATCH] D54526: [AST] Pack BinaryOperator

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno marked an inline comment as done.
riccibruno added a comment.

Added some inline comments.



================
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; }
----------------
dblaikie wrote:
> Lost LLVM_READONLY somewhere along the way? Should it be added back in?
On purpose. `LLVM_READONLY` on a simple getter seems pointless,
since the compiler is just going to inline it.


================
Comment at: include/clang/AST/Expr.h:3259
+  }
+  bool isPtrMemOp() const { return isPtrMemOp(getOpcode()); }
+
----------------
I added it for consistency with the other similar function below.
`isPtrMemOp` is the only one without the pattern of having
a static member function and a member function calling it.
I would not expect this to matter for performance since the compiler
is just going to see right through the `getOpcode` call.


================
Comment at: include/clang/AST/Stmt.h:572
     CastExprBitfields CastExprBits;
+    BinaryOperatorBitfields BinaryOperatorBits;
     InitListExprBitfields InitListExprBits;
----------------
dblaikie wrote:
> Oh, just a thought - wonder if we could/should have an assert that this anonymous union doesn't grow beyond a certain intended size? (can we have such an assert? I'm not sure how to test the size of an anonymous union - I guess we could static_assert the size of the whole Stmt class, though that'd be a bit brittle)
Already done in line 657 below. And yes it is a valid concern !
When I did the same thing for the types hierarchy I spotted
that the bit-field class `FunctionTypeBitfields` was taking more
than 32 bits accidentally. (fixed since) Oops.


Repository:
  rC Clang

https://reviews.llvm.org/D54526