<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 9, 2014 at 3:04 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This patch addresses the first stage of PR17891 by folding constant<br>
arguments together into a single MDString.  Integers are stringified and<br>
a `\0` character is used as a separator.<br>
<br>
----<br>
<br>
Known defects:<br>
<br>
  - I've only barely started to update testcases.<br>
  - There are bound to be bugs ~~~~~~~~~~~~^<br>
  - I deleted dead code (I'll separate that out before committing).<br>
<br>
I'd really appreciate some early feedback on this nevertheless.  I'm<br>
finding the testcases painful enough to update that I don't want to do it<br>
twice.<br>
<br>
 1. I think `\0` is cleaner than `,`.  Bike shed?<br></blockquote><div><br></div><div>OK by me, but depending on how much schema and test case churn you're inclined to (I know you folks at Apple are a bit more concerned about schema churn than I am - because you have static bitcode archives, etc) we might want to discuss the whole thing in a bit more detail. Things like:<br><br>* how's the scheme going to extend to hierarchical data? (eventually we'll hopefully have rolled most of the whole DIE tree into single big strings - separate out the types (which are still huge/non-trivial with all the member function declarations, etc), etc)<br>* Do we want to consider a more human readable form (perhaps with an internal clang flag to force it to produce that version of the metadata?) for the purpose of writing test cases? Perhaps self-describing, so we can more easily omit irrelevant fields from tests (when the file/line/col doesn't matter for a particular declaration, etc).<br><br>If you're willing to churn this more later, I don't mind deferring these decisions, if not - we might want to plan a bit more carefully.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 2. I'm using hex for the tags.  Good idea?<br></blockquote><div><br>Probably - that's the way they're written in the DWARF standard (& hopefully in our Dwarf.h too).<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 3. I didn't add a prefix, which David suggested just now in PR17891.<br>
    This would be the time to do it, but is the hex tag sufficient?<br></blockquote><div><br></div><div>The reason for the suggestion was to generalize the metadata annotation scheme a bit, but we don't have anything to generalize over yet. If we're willing to have the false positives of trying to render metadata annotations on any other metadata that starts with a string starting with "0x.." (we can/are a bit more precise than that - checking it has the right number of fields, etc), etc... then no big deal.<br><br>Again, we're no worse off with what you've got here than what we have today - and some time in the future we might want to add a consistent/more general schema. *shrug*</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 4. Anything (other than testcases) I've missed updating?<br>
<br>
 5. Do you have specific advice on automating the testcase update flow?<br>
    My next step is building tool support, so suggestions (and starter<br>
    scripts) are welcome.<br>
<br>
</blockquote></div><br>HeaderTwine doesn't look like a Twine - it looks like a string builder of sorts. (I'd avoid using the term twine for something that doesn't do the magic delayed string concatenation that llvm's twine does) maybe HeaderString?</div><div class="gmail_extra"><br></div><div class="gmail_extra">Some of the Verify implementations look off - old num operands == new num operands + header fields - 1 (minus one for the header itself being counted in the number of operands), right? Some don't add up (DITemplateTypeParameter::Verify went from 7 to 4 + 3, so isn't it missing a field?<br><br>Any size measurements/test results (does this bootstrap?) on this?<br><br><br></div></div>