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

[Zach Wheeler]

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?

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110701/e3603d3f/attachment.html>