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

[Eric Christopher]

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