[PATCH] D50050: [AST] CastExpr: BasePathSize is not large enough.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: include/clang/AST/Expr.h:2791
+public:
+  using BasePathSizeTy = unsigned short;
+  static_assert(std::numeric_limits<BasePathSizeTy>::max() >= 16384,
----------------
I'll let @rjmccall comment here, but I think I'd be willing to give up 2 more bytes here and just go with unsigned int.  It has the advantage of likely never being an issue again (since 4 billion bases is not an issue).

We might be paying for those 2 ANYWAY with alignment as well, right?


================
Comment at: include/clang/AST/Expr.h:2800
 
+  BasePathSizeTy BasePathSize() const {
+    if (BasePathIsEmpty())
----------------
Looking back up, I doubt the need for this either, since we have path_empty and path_size below.


================
Comment at: include/clang/AST/Expr.h:2805
+  }
+  BasePathSizeTy &BasePathSize();
+
----------------
Why is this a reference?  This seems odd... It seems that the const-cast magic above shouldn't be necessary either.


================
Comment at: include/clang/AST/Expr.h:2814
+    assert(!BasePathIsEmpty() && basePathSize != 0);
+    BasePathSize() = basePathSize;
+    assert(BasePathSize() == basePathSize &&
----------------
This is a super weird way to do this. I really am not a fan of giving a reference to that spot instead of just setting it here.  


================
Comment at: include/clang/AST/Expr.h:2854
 
+  bool BasePathIsEmpty() const { return CastExprBits.BasePathIsEmpty; }
+
----------------
Is this necessary?  Shouldn't path_empty just do this?


================
Comment at: test/CodeGenCXX/castexpr-basepathsize-threshold.cpp:1
+// RUN: %clang_cc1 %s -emit-llvm -o -
+
----------------
lebedev.ri wrote:
> erichkeane wrote:
> > First, most of this test can be further simplified.  Second, I'd like to see a test that actually tests the limit without hitting the recursive template limit.
> > First, most of this test can be further simplified.
> OK? This is what creduce gave me. But yes, i know it has some issues with templates.
> 
> > Second, I'd like to see a test that actually tests the limit without hitting the recursive template limit.
> But that is true already. *this* test does not hit the recursive template limit.
Looking again, it is probably reduced sufficiently.  The cast and the recurse-depth is the only things that are important, so I guess I'm OK with this.


Repository:
  rC Clang

https://reviews.llvm.org/D50050