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

[Puyan Lotfi]

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.
>>