[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In https://reviews.llvm.org/D54166#1299230, @rjmccall wrote:

> In https://reviews.llvm.org/D54166#1299149, @riccibruno wrote:
>
> > In https://reviews.llvm.org/D54166#1298889, @rjmccall wrote:
> >
> > > IIRC, abbreviations just silently don't take effect if the record doesn't conform; so things will appear to work, but the size on disk will be bigger.
> >
> >
> > I looked at where the abbreviations are defined, and it seems that the only abbreviations for
> >  statements/expressions are for `DeclRefExpr`, `IntegerLiteral`, `CharacterLiteral`
> >  and `ImplicitCastExpr` (grep for `EmitAbbrev` in `Serialization/`,
> >  for some reasons they are emitted in `WriteDeclAbbrev()`...).
> >
> > And indeed changing the serialization format of `CharacterLiteral` triggers various assertions
> >  because of the abbreviation. Therefore unless I am missing something no other statement/expression
> >  has currently an abbreviation. I suspect therefore that someone could go wild and cut the on-disk
> >  size of the serialization format significantly here.
> >
> > I looked at the size of the generated pch for all of Boost, and I am only seeing an increase of
> >  about 8k, which is entirely attributable to the fact that I am adding one field to the serialization
> >  format. I can rework it to remove this additional field if needed.
>
>
> If you're generally interested in improving build times, working on the serialization format would probably be really valuable.  We don't generally insist on a zero-regressions policy in other commits, though.


I was planning to finish packing the other expression classes to minimize the in-memory footprint
of the AST first. I will try to take a look at serialization if I have some time but no promise for now.
Thanks for the review !


Repository:
  rC Clang

https://reviews.llvm.org/D54166