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

[Puyan Lotfi]

On Oct 4, 2013, at 10:43 AM, Chandler Carruth <chandlerc at google.com> wrote:

> 
> On Fri, Oct 4, 2013 at 1:39 PM, Puyan Lotfi <plotfi at apple.com> wrote:
> I like this solution but do you think there should be some way to tell the library (optional parameter, or a separate API function) to use the argv data directly instead of allocating memory on the context. Seems like it would be good to not hurt performance of the common case.
> 
> I don't think there is a common case where performance is a concern. Why?
> 
> 1) We're parsing commandline flags in an inherently race-prone way, and for flags which are explicitly for debugging and custom manipulations only.
> 2) It can only happen once per LLVMContext spin up / tear down
> 

I mean the common case where ParseCommandLineOptions is being called from main and being passed argv directly. Wouldn’t it be better to not allocate duplicates inside LLVMContext in this case? But yeah, otherwise I like the idea of putting strings into LLVMContext.

> If these options really need to be controlled by library users in the common case, they *need* to be lifted up to proper APIs on the passes or PassManagerBuilder. The flags interface is for "opt", debugging, experimentation, etc. Currently, the only users I know of who abuse this are Clang itself for several backend options, but these are *bugs* that need to be fixed, and so I'm not at all worried about their performance.
>  
> 
> > Without doing something to take ownership of the memory when parsing flags, this change will break a very large number of users of LLVM, so I don't think there is going to be a significantly simpler solution here.
> >
> 
> I think this could work. Do you think a persistent uniquing string pool (StringMap of char*’s) in LLVMContext would be in order?
> 
> I don't think a StringMap makes much sense. There is no string key here. I don't think you even need to do uniquing, because it just doesn't seem like performance is going to matter here.
> 
> I would use a DenseMap<cl::opt<StringRef>*, const char*> or some such... I wish we had a nice unique_ptr to store as the value type, but whatever.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131004/e2e49248/attachment.html>