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