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

[Zachary Turner]

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?

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, 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)

On Wed, Feb 25, 2015 at 10:24 AM Enrico Granata <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> 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
>
> EMAIL PREFERENCES
>  http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
>
> Thanks,
> *- Enrico*
> 📩 egranata@.com ☎️ 27683
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150225/a42e76d8/attachment.html>