[PATCH] DI: Fold constant arguments into a single MDString

[Duncan P. N. Exon Smith]

> On 2014-Sep-25, at 10:39, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Fri, Sep 19, 2014 at 5:29 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> > This patch addresses the first stage of PR17891 by folding constant
> > arguments together into a single MDString.  Integers are stringified and
> > a `\0` character is used as a separator.
> 
> I've attached updated LLVM and clang patches, rebased this afternoon
> with all tests in `check-all` passing.  I believe it addresses David's
> comments as well.
> 
> There are an insane number of changes in the testcases, but changing the
> schema of the dwarf tags makes that inevitable -- almost every line of
> metadata had to change.  I look forward to "stage 2" of PR17891, which
> can be more incremental.
> 
> Let me know if the updated patches look good.
> 
> General idea looks good - might be easier to do the micro-review stuff in a Phab review

Perhaps this outs me as a backwards techno-fool, but I find phab makes
reviews harder for me to follow.  I think micro-review is better done
post-commit anyway.

> (some things I saw: 
> * DIHeaderIterator could probably just be a generic StringIterator (could be templated on a delimiter or just hardcoded in the form of a NullSeparatedStringIterator?)? - again, using the DI prefix for something not in the DI* hierarchy is something I twitch about a bit... though DIRef, etc, are precedent in favor of your naming there

I think you're looking at an old version of the patch.  The version
that Eric LGTM'ed is "*-v4.patch" and calls this
`DIHeaderFieldIterator`.

Sorry this isn't in tree yet -- it would be easier for you to look at
there, but I'm waiting for an LGTM on Adrian's dbg.value intrinsics
patch (if he goes first, I need to rerun my testcase scripts; if I go
first, he needs to *rewrite* his testcase scripts.)

On topic: `NullSeparatedStringIterator` could work.  I like
`DIHeaderFieldIterator`, since it's self-documenting, but I'm not too
fussed about the name (it's an implementation detail that I'd rather
wasn't leaked at all).  If you feel strongly I'll change it though.

> * getHeaderFields could probably return a range rather than having to build an entire vector

Actually, this code is dead (!?) so I've removed it.

> * getHeaderField(unsigned) could probably use std::advance

All of the DI-hierarchy accessors provide a `StringRef()` for an
out-of-bounds access (or a `0` for integer accesses).  This patch
maintains that behaviour.

Using `std::advance()` here would iterate past the end when there
aren't enough header fields, firing an assertion in
`DIHeaderFieldIterator::increment()`.  This is how I originally wrote
the code, and it was great for shaking out bugs.

Nevertheless, I'd rather keep this patch as close to NFC as I can,
so if you want to change the behaviour I'd prefer that to be separate.

For now, I've added a comment explaining why `std::advance()` doesn't
work.

> * DIHeader would go well with the StringIterator above, mabye NullSeparatedStringBuilder? It sort of feels like a builder-y thing... 

"v4.patch" calls this `HeaderBuilder`, which fixes the big issue with
the previous name.  Personally, I think having "Header" in the name
makes the code more clear, but if you feel strongly I'll change it to
what you like.

> * Why the "Twinish" perfect forwarding in DIHeader::concat? Rather than just taking a Twine?

This reduces complexity in the caller.  Many of `Twine`'s constructors
are explicit.  In the context of `HeaderBuilder::concat()`, converting
to a `Twine` is unsurprising, so I chose to reduce boilerplate.

However, `Twinish` is a horrible name.  I've renamed it `Twineable`.

> * I guess the builder could be just a variadic function one day... *sigh*

Some day, FTW :).

> I think the notion was to do that once per schema-changing LLVM open source release. Otherwise you'd have to change it on every schema change and that involves changing every debug info metadata test file :/ (& I'd really like us not to churn it /that/ much if we can help it)


Since we've had a release and I'm touching every debug info metadata
test file *anyway*, I've bumped it to "2".

Updated and rebased patches attached.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-DI-Header-MDString-llvm-v5.patch
Type: application/octet-stream
Size: 2195156 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140925/1e05268b/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-DI-Header-MDString-clang-v5.patch
Type: application/octet-stream
Size: 93530 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140925/1e05268b/attachment-0001.obj>