Looking at DiagnosticIDs.cpp again, it seems that he probably didn't store straight StringRefs because they would not pack efficiently for the short strings he was anticipating. <div><br></div><div>OK, I'm not following you. The compilation of /which file/ gets slowed down by constructor calls to /what object/, exactly?</div>
<div><br></div><div>ZJ<br><br><div class="gmail_quote">On Fri, Jul 1, 2011 at 2:42 PM, Matthieu Monrocq <span dir="ltr"><<a href="mailto:matthieu.monrocq@gmail.com">matthieu.monrocq@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div></div><div class="h5"><br><br><div class="gmail_quote">2011/7/1 Zach Wheeler <span dir="ltr"><<a href="mailto:zachw.foss@gmail.com" target="_blank">zachw.foss@gmail.com</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I would suggest that a comment be added to Option.h or Option.cpp indicating that the existing pattern is intentional and giving a brief explanation of why. I can submit a patch to do that, if you want.<div><br></div><div>
Could the length of the string be stored in Option so that strlen is not needed?</div><div><br></div><div>Does clang::driver::ArgList::getArgString() return const char* for the same reason?</div><div><br></div><div>ZJ<div>
<div></div><div><br>
<br><div class="gmail_quote">On Fri, Jul 1, 2011 at 1:22 PM, Matthieu Monrocq <span dir="ltr"><<a href="mailto:matthieu.monrocq@gmail.com" target="_blank">matthieu.monrocq@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Message: 3<br>
Date: Fri, 1 Jul 2011 12:09:04 -0400<br>
From: Zach Wheeler <<a href="mailto:zachw.foss@gmail.com" target="_blank">zachw.foss@gmail.com</a>><br>
Subject: Re: [cfe-commits] [PATCH] StringRef'ize<br>
clang::driver::Option::getName()<br>
To: Chris Lattner <<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>><br>
Cc: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
Message-ID: <<a href="mailto:BANLkTimoXHdSP9Vz0mStwt_xHtORoPtqwQ@mail.gmail.com" target="_blank">BANLkTimoXHdSP9Vz0mStwt_xHtORoPtqwQ@mail.gmail.com</a>><br>
Content-Type: text/plain; charset="iso-8859-1"<br>
<br>
I forgot to initialize NameLen. :-/<br>
<br>
I stored 'Name' as start+size because I saw that pattern in<br>
lib/Basic/DiagnosticID.h, and assumed it was expected. Storing a StringRef<br>
makes more sense to me. Here's an updated patch. Clang builds successfully,<br>
and hopefully the tests will pass.<br>
<br>
ZJ<br>
<br>
On Thu, Jun 30, 2011 at 4:04 PM, Chris Lattner <<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>> wrote:<br>
<br>
><br>
> On Jun 30, 2011, at 11:47 AM, Zach Wheeler wrote:<br>
><br>
> > This patch changes the return type of Option::getName() from const char*<br>
> to StringRef, doing away with the need for several calls to strlen.<br>
> > Clang builds fine with these changes.<br>
><br>
> THis patch looks reasonable, except that it completely breaks the testsuite<br>
> :). Please track down the bug and resubmit a patch. Also, why not just<br>
> embed a 'Name' StringRef in Option.h instead of storing the start+size?<br>
><br>
> -Chris<br>
><br>
><br>
-------------- next part --------------<br>
An HTML attachment was scrubbed...<br>
URL: <a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20110701/782f855a/attachment-0001.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20110701/782f855a/attachment-0001.html</a><br>
-------------- next part --------------<br>
A non-text attachment was scrubbed...<br>
Name: StringRef-ize--driver.Option.getName-2.patch<br>
Type: application/octet-stream<br>
Size: 5033 bytes<br>
Desc: not available<br>
Url : <a href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20110701/782f855a/attachment-0001.obj" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20110701/782f855a/attachment-0001.obj</a><br>
</blockquote></div><br>Hi Zach,<br><br>The pattern in DiagnosticIDs.cpp comes from Argyrios :)<br><br>I tried using StringRef first, unfortunately it led to a massive explosion of the compilation time of the file because the compiler had to emit *lots* of constructor calls, while "bare" C types are trivial to initialize. Argyrios tracked the problem down and came up with this solution, it's a bit gross I think (and unfortunate) but noone could propose a better solution that did not involve calls to strlen at runtime.<br>
<br>This pattern is an optimization, and not meant to be reproduced. Sorry for it being misleading :/<br><font color="#888888"><br>-- Matthieu<br><br>
</font></blockquote></div><br></div></div></div>
</blockquote></div></div></div>As far as I can tell, using two separate arguments (char const* and size_t) is specific to DiagnosticIDs.cpp (remember that we are talking about static variables here).<br><br>Other uses should probably be better served, in general, by a llvm::StringRef (especially those with a bare char const*).<br>
<br>I therefore *suppose* that Option and ArgList could use llvm::StringRef, but don't take my word for it: I am only 2 or 3 patches ahead of you, so definitely no expert on the code base :)<br><font color="#888888"><br>
-- Matthieu<br>
</font></blockquote></div><br></div>