mstorsjo added inline comments.
================
Comment at: lib/Driver/Job.cpp:104-105
+ std::string Buf;
+ if (PathStyle == llvm::sys::path::Style::posix) {
+ Buf = llvm::sys::path::convert_to_slash(Arg);
+ Arg = Buf;
----------------
rnk wrote:
> mstorsjo wrote:
> > rnk wrote:
> > > This is blindly treating every argument as if it were a path. I can
> > > imagine someone wanting to define a macro to a string value containing
> > > backslashes, such as: `-DAWESOME_MACRO="\"asdf\""`
> > > Given that this is used for -### and crash diagnostic printing, I'm
> > > hesitant to do this here. I think it might be better to, in a cygming
> > > environment, try to convert all our paths to forward slashes before we
> > > add them to the command line.
> > A quick test of the concept is at
> > http://martin.st/temp/clang-forward-slash.patch; this seems to be roughly
> > enough to get forward slashes for all paths in the linker command at least.
> > Does this look like what you have in mind?
> >
> > For a plain "clang -v test.c", this takes care of everything except the
> > temporary object file. It doesn't do anything for the compile command
> > though. This should be roughly the minimal amount needed for this
> > particular case of libtool, but then again makes the -v output wildly
> > inconsistent.
> >
> > Changing the path separator earlier would kinda require changing the
> > preferred path separator throughout all callsites to any path api, or
> > change the path utils to allow overriding the global default somehow.
> I think our output is already pretty inconsistent. Most build systems use
> forward slash paths internally, so that's what the compiler sees on the
> command line. Then, we dutifully take those paths and join them with the
> "preferred" separator (`\`), and make paths with mixed slashes, which nobody
> wants.
>
> I would go with the patch you put forward, if that solves the issue.
> Everything there is very gnu toolchain oriented, it makes sense to try to
> match their path style.
>
> The one change I might object to is `ToolChain::GetProgramPath`, since cmd
> really wants the name of the program to have back slashes. As long as that
> comes out with backslashes in -### and crash report .sh files, I'd be OK with
> it.
Ok, thanks - I'll test run things with that patch (minus the executable change)
and update this review properly later then.
Repository:
rC Clang
https://reviews.llvm.org/D53066
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
