> On Feb 25, 2015, at 10:29 AM, Zachary Turner <ztur...@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 <egran...@apple.com > <mailto:egran...@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 <ztur...@google.com >> <mailto:ztur...@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@cs.uiuc.edu <mailto:lldb-commits@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
_______________________________________________ lldb-commits mailing list lldb-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
