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

[Zach Wheeler]

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