[Lldb-commits] [PATCH] Move the shell expansion code to Host/common

[Enrico Granata]

> On Feb 25, 2015, at 10:29 AM, Zachary Turner <zturner at google.com> wrote:
> 
> Well, I agree in principle that the API is meant to support process launching.  But if its utility can be trivially extended to support other use cases, then why not?
> 

YAGNI - also, ProcessLaunchInfo isn’t that hard to construct

> FWIW, when I decided to do this, it was only to enable the functionality for other platforms (i.e. moving from specific dirs to common), because only Windows is going to have a different codepath here so it didn't make sense to increase the code debt for everyone just because of Windows.  Then changing of the signature was just something I noticed on the side.  I can change it back if people feel strongly,

Please do
FWIW, I think that the environment variables in the launch info should ALSO be passed down (they aren’t now, but it’s not by design), so with your change, I’d have to go add yet another argument when I have time to get that piece of work done - and by then, I am passing essentially all the guts of a ProcessLaunchInfo, but not the actual ProcessLaunchInfo for no clear reason

> but at the same time, I really like making code as general as possible, as long as the generality doesn't hinder the readability or usability for the primary use case (which in this case I don't think it does)

I don’t think I agree with that (i.e., I think it does make our real use case for this less readable and usable)

> 
> On Wed, Feb 25, 2015 at 10:24 AM Enrico Granata <egranata at apple.com <mailto:egranata at apple.com>> wrote:
> -    
> -    void
> -    SetShellExpandArguments (bool glob);
> -    
> +
> +    void SetShellExpandArguments(bool expand);
> +
> 
> Yes, totally
> 
> -    static Error
> -    ShellExpandArguments (ProcessLaunchInfo &launch_info);
> -    
> +    static Error ShellExpandArguments(llvm::StringRef input, llvm::StringRef working_dir, std::vector<std::string> &expanded);
> +
> 
> Why? I see no benefit to doing this. This API is clearly meant to support process launching. This change seems a net loss to me.
> 
> The actual moving of the argdumper logic to Host/common is fine (as long as the mechanism for individual platforms to opt out is trivial, that is) - the change in signature from ProcessLaunchInfo to bunch-of-stuff not so much
> 
> -    if (launch_info.GetFlags().Test(eLaunchFlagShellExpandArguments))
> 
> This should be fine do to do, yes
> 
> 
>> On Feb 24, 2015, at 7:31 PM, Zachary Turner <zturner at google.com <mailto:zturner at google.com>> wrote:
>> 
> 
>> Anyone have thoughts on this?  If there's no suggestions I'd like to commit, but I'll give another day or two for comments.
>> 
>> 
>> http://reviews.llvm.org/D7805 <http://reviews.llvm.org/D7805>
>> 
>> EMAIL PREFERENCES
>>  http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
>> 
>> 
>> 
> 
>> _______________________________________________
>> lldb-commits mailing list
>> lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits <http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits>
> 
> Thanks,
> - Enrico
> 📩 egranata@.com ☎️ 27683
> 
> 
> 
> 

Thanks,
- Enrico
📩 egranata@.com ☎️ 27683




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150225/32f9c0b7/attachment.html>