[PATCH] Allow for the use of StringRef command line options instead of cl::opt<std::string>

[Puyan Lotfi]

Hi All

I’ve improved this StringRef patch and I’d like some more feedback and hopefully a submission.
My changes to the original patch thus far have been:

- I added changes to CreateTargerMachine() in clang to have the SmallVector passed in from the caller.
- I found more places where cl::opt<str::string> was used, and I changed them to use cl::opt<StringRef>.

I’ve attached a patch that should apply cleanly with an llvm checkout that includes clang checked out into the tools directory. 

Thanks

-Puyan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: cloptstrrefwclang.patch
Type: application/octet-stream
Size: 52245 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130919/685c1496/attachment.obj>
-------------- next part --------------


On Aug 29, 2013, at 4:45 PM, Eric Christopher <echristo at gmail.com> wrote:

> Probably best for now.
> 
> -eric
> 
> On Thu, Aug 29, 2013 at 4:03 PM, Puyan Lotfi <plotfi at apple.com> wrote:
>> Excellent. I’ll try that. Should I send a separate patch for review to the cfe-commits list for this tiny change?
>> 
>> -Puyan
>> 
>> On Aug 27, 2013, at 11:42 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>>> On Sun, Aug 25, 2013 at 3:32 PM, Puyan Lotfi <plotfi at apple.com> wrote:
>>>> Should this be put on hold until we have a better solution than replacing cl::opt<std::string> with cl::opt<StringRef>? I took another look at all the places that where ParseCommandLineOptions is not called with argc/argv from main. I found:
>>>> 
>>>> clang/tools/driver/cc1as_main.cpp
>>>> clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
>>>> clang/lib/CodeGen/BackendUtil.cpp
>>>> clang/lib/Tooling/CommonOptionsParser.cpp
>>>> 
>>>> Among these cc1as_main.cpp and ExecuteCompilerInvocation.cpp leak memory regardless of the changes. CommonOptionsParser.cpp uses ParseCommandLineOptions but is called from unittests I already know are passing. Finally BackendUtil.cpp is still of concern if the options parsed are used by EmitAssemblyHelper::CreateTargetMachine()'s callers. I think a solution to this could be to move SmallVector<const char *, 16> BackendArgs; into the global scope as a work around and clear it out every time it is used when calling ParseCommandLineOptions.
>>> 
>>> Rather than moving this globally, I'd move it up one layer, to
>>> CreateTargetMachine's caller (& owner/lifetime manager of the
>>> resulting TargetMachine object), EmitAssembly. That should give it the
>>> required lifetime while remaining re-entrant, thread-safe, etc.
>>> 
>>>> 
>>>> Outside of this, it could just be a better solution to work out a way to not rely on static ctors to begin with.
>>> 
>>> I think you're on the right track with this.
>>> 
>>>> 
>>>> Let me know your thoughts. Thanks.
>>>> 
>>>> -Puyan
>>>> 
>>>> 
>>>> On Aug 16, 2013, at 8:54 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>> 
>>>>> On Fri, Aug 16, 2013 at 8:43 AM, Chris Lattner <clattner at apple.com> wrote:
>>>>>> 
>>>>>> On Aug 15, 2013, at 10:57 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>> 
>>>>>>> On Thu, Aug 15, 2013 at 4:56 PM, Puyan Lotfi <plotfi at apple.com> wrote:
>>>>>>>> 
>>>>>>>> This sounds like a workable compromise.
>>>>>>>> To avoid leaking how about a CommandLine.cpp handled string pool that allocates copies on demand (specified maybe by an optional flag to ParseCommandLineOptions/ParseEnvironmentOptions) but also has the option of flushing/deallocating the pool?
>>>>>>> 
>>>>>>> I'm with Chris - I think it should be simpler to have
>>>>>>> ParseCommandLineOptions/CommandLine in general not be concerned with
>>>>>>> task of owning the strings. That should be the responsibility of the
>>>>>>> caller.
>>>>>> 
>>>>>> Also, I don't know if switching from cl::opt<std::string> to cl::opt<StringRef> is enough, but I'd really like for the cl::opts scattered through llvm to stop requiring static constructor/destructors.  This seems like a useful step along the way.
>>>>> 
>>>>> True - it shouldn't be hard to make StringRef's defoult ctor constexpr
>>>>> under C++11 - not sure what else is in a cl::opt that might present
>>>>> challenges to that kind of solution.
>>>> 
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits