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

[David Blaikie]

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