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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Its not that it needs to be 'at least 9 bits', IMO 9 bits was too small as well.

What I'd prefer to this solution is similar to what John McCall said on PR38356: Add the size to the trailing allocated space.  Check out the 'create' methods for all of these, and make the first sizeof(void*) bytes be the size.  I'd also suggest using 1 bit in CastExprBits to be a 'is base path empty', so that you don't have to allocate for it at all.



================
Comment at: include/clang/AST/Expr.h:2796
+  unsigned BasePathSize : 14;
+
   bool CastConsistency() const;
----------------
The comment there is wrong.  It was wrong at 9 bits as well, since it cannot handle our implimits.  I'd like to see if you can find where the implimit for direct/indirect bases is stored, and write a static-assert for it here.


================
Comment at: test/CodeGenCXX/castexpr-basepathsize-threshold.cpp:1
+// RUN: %clang_cc1 %s -emit-llvm -o -
+
----------------
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.


================
Comment at: test/CodeGenCXX/castexpr-basepathsize-threshold.cpp:20
+  // not fit it, so we are testing that we do fit it.
+  // If -ftemplate-depth= is provided, larger values (4096 and up) cause crashes
+  // elsewhere.
----------------
Shouldn't we cover 16k?  


Repository:
  rC Clang

https://reviews.llvm.org/D50050