riccibruno marked an inline comment as done.
riccibruno added inline comments.
================
Comment at: include/clang/AST/Expr.h:3029
+ assert((CastExprBits.BasePathSize == BasePathSize) &&
+ "BasePathSize overflow!");
assert(CastConsistency());
----------------
riccibruno wrote:
> lebedev.ri wrote:
> > riccibruno wrote:
> > > lebedev.ri wrote:
> > > > rjmccall wrote:
> > > > > riccibruno wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > riccibruno wrote:
> > > > > > > > It cannot overflow now, but someone in the future is going
> > > > > > > > to shrink `CastExprBitfields::BasePathSize`.
> > > > > > > Can you move that static assert here instead of deleting it?
> > > > > > I am not sure it is really useful since when someone will want to
> > > > > > shrink it, it will be stored as `unsigned BasePathSite : x;`. What
> > > > > > we
> > > > > > don't want is having `x` too small. However `numeric_limits` will
> > > > > > be based on the underlying type of the bit-field and so will never
> > > > > > fail.
> > > > > >
> > > > > > Storing it as something other than an unsigned, say a short, would
> > > > > > work,
> > > > > > but it will pack poorly with MSVC.
> > > > > >
> > > > > > Hopefully the comment in `CastExprBitfields` will tell people to
> > > > > > not shrink it
> > > > > > mindlessly.
> > > > > I agree that the type-based numeric limit is not useful if we're
> > > > > anticipating
> > > > > it becoming a bit-field. The comment seems fine since it's in the
> > > > > code that
> > > > > would need to be edited.
> > > > I'm just worried that now there will be less of a test coverage,
> > > > The actual run-time test already does not test template instantiation
> > > > with depth of `16384`, but much less.
> > > Would it make sense to go systematically through the limits recommended by
> > > [implimits], and for each limit devise a test which will exercise it ?
> > >
> > > Or would this take too long to be a test ?
> > >
> > > It would perhaps also be nice to document these limits somewhere,
> > > even if it is just "You can rely on [implimits]".
> > > Would it make sense to go systematically through the limits recommended by
> > > [implimits], and for each limit devise a test which will exercise it ?
> >
> > Better test coverage is always good.
> >
> > > Or would this take too long to be a test ?
> >
> > The problem is, even this you can't really test with a run-time test.
> > Template instantiation is recursive, so in most environments you overflow
> > your stack long before you reach the recommended depth of `16384`.
> >
> But you can perhaps test this without doing any template instantiation.
> The limit is "Direct and indirect base classes [16384]". Therefore you can
> just construct 2^16 classes with macros which should not be recursive,
> and then somehow construct a class with all of these classes as a base
> (although this step is perhaps recursive).
I meant obviously 2^14
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56358/new/
https://reviews.llvm.org/D56358
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
