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

[Matthieu Monrocq]

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/62f003c7/attachment.html>