[cfe-commits] cfe-commits Digest, Vol 49, Issue 1

[Argyrios Kyrtzidis]

On Jul 1, 2011, at 12:28 PM, Zach Wheeler wrote:

> 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. 
> 
> OK, I'm not following you. The compilation of /which file/ gets slowed down by constructor calls to /what object/, exactly?

DiagnosticIDs.cpp contains huge static arrays. Putting a StringRef into the struct there would make the struct non-POD and its constructor would have to be called, and since it is a global array all the constructor calls would occur in the global initializer function.
That caused a huge slow down when compiling that file but, apart from that, we generally try to avoid the need for global initializer functions as much as possible.

Putting a StringRef into a class that is not going to be on a global variable is highly recommended though :-)

-Argyrios

> 
> ZJ
> 
> On Fri, Jul 1, 2011 at 2:42 PM, Matthieu Monrocq <matthieu.monrocq at gmail.com> wrote:
> 
> 
> 2011/7/1 Zach Wheeler <zachw.foss at gmail.com>
> 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.
> 
> Could the length of the string be stored in Option so that strlen is not needed?
> 
> Does clang::driver::ArgList::getArgString() return const char* for the same reason?
> 
> ZJ
> 
> 
> On Fri, Jul 1, 2011 at 1:22 PM, Matthieu Monrocq <matthieu.monrocq at gmail.com> wrote:
> 
> 
> Message: 3
> Date: Fri, 1 Jul 2011 12:09:04 -0400
> From: Zach Wheeler <zachw.foss at gmail.com>
> Subject: Re: [cfe-commits] [PATCH] StringRef'ize
>        clang::driver::Option::getName()
> To: Chris Lattner <clattner at apple.com>
> Cc: cfe-commits at cs.uiuc.edu
> Message-ID: <BANLkTimoXHdSP9Vz0mStwt_xHtORoPtqwQ at mail.gmail.com>
> Content-Type: text/plain; charset="iso-8859-1"
> 
> I forgot to initialize NameLen. :-/
> 
> I stored 'Name' as start+size because I saw that pattern in
> lib/Basic/DiagnosticID.h, and assumed it was expected. Storing a StringRef
> makes more sense to me. Here's an updated patch. Clang builds successfully,
> and hopefully the tests will pass.
> 
> ZJ
> 
> On Thu, Jun 30, 2011 at 4:04 PM, Chris Lattner <clattner at apple.com> wrote:
> 
> >
> > On Jun 30, 2011, at 11:47 AM, Zach Wheeler wrote:
> >
> > > This patch changes the return type of Option::getName() from const char*
> > to StringRef, doing away with the need for several calls to strlen.
> > > Clang builds fine with these changes.
> >
> > THis patch looks reasonable, except that it completely breaks the testsuite
> > :).  Please track down the bug and resubmit a patch.  Also, why not just
> > embed a 'Name' StringRef in Option.h instead of storing the start+size?
> >
> > -Chris
> >
> >
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20110701/782f855a/attachment-0001.html
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: StringRef-ize--driver.Option.getName-2.patch
> Type: application/octet-stream
> Size: 5033 bytes
> Desc: not available
> Url : http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20110701/782f855a/attachment-0001.obj
> 
> Hi Zach,
> 
> The pattern in DiagnosticIDs.cpp comes from Argyrios :)
> 
> 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.
> 
> This pattern is an optimization, and not meant to be reproduced. Sorry for it being misleading :/
> 
> -- Matthieu
> 
> 
> 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).
> 
> Other uses should probably be better served, in general, by a llvm::StringRef (especially those with a bare char const*).
> 
> 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 :)
> 
> -- Matthieu
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110701/977b2c62/attachment.html>