[PATCH] D56358: [AST] Move back BasePathSize to the bit-fields of CastExpr

[Bruno Ricci via Phabricator via cfe-commits]

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());
----------------
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]".


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56358/new/

https://reviews.llvm.org/D56358