smeenai created this revision. smeenai added reviewers: compnerd, hans, mstorsjo, rnk, thakis, zturner. Herald added a project: clang. Herald added a subscriber: cfe-commits.
Escaping backslashes results in unnatural looking output, and the actual escapes are mostly unnecessary. We were also not consistent about when we escaped backslashes and when we didn't, making it impossible to e.g. store the InstalledDir output to a FileCheck variable and then use that variable to check paths relative to the driver. This change could be generalized to any platform which uses the backslash as a path separator instead of just Windows. I'm unaware of other such platforms, however, and I'm also unsure if not escaping the backslash would be appropriate for those platforms, so I'm limiting to Windows for now. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D64538 Files: clang/lib/Driver/Job.cpp clang/test/Driver/backslash.c Index: clang/test/Driver/backslash.c =================================================================== --- /dev/null +++ clang/test/Driver/backslash.c @@ -0,0 +1,6 @@ +// RUN: %clang -fsyntax-only %s -### 2>&1 | FileCheck %s +// Ensure the treatment of backslashes is consistent in the printed InstalledDir +// and commands. We would previous escape backslashes in the printed commands +// but not in the printed InstallDir, causing the match to fail. +// CHECK: InstalledDir: [[INSTALLEDDIR:.+$]] +// CHECK: "[[INSTALLEDDIR]]{{[/\\]}}clang Index: clang/lib/Driver/Job.cpp =================================================================== --- clang/lib/Driver/Job.cpp +++ clang/lib/Driver/Job.cpp @@ -107,9 +107,26 @@ } // Quote and escape. This isn't really complete, but good enough. + // On Windows, the backslash is the path separator, and escaping the backslash + // results in unnatural-looking output that's also inconsistent with how paths + // are printed out elsewhere (e.g. when printing out the installation + // directory). Windows native shells (cmd and PowerShell) use different escape + // characters so escaping the backslash is unnecessary. bash (the most common + // non-native shell on Windows) uses backslash as an escape character inside + // double quotes only when it's followed by $, `, ", \, or a literal newline; + // the last three are disallowed in paths on Windows and the first two are + // unlikely, so this shouldn't cause issues in practice. Note that we always + // enclose an argument containing backslashes in quotes even if we aren't + // escaping the backslashes, since bash always interprets it as an escape + // character outside quotes. +#if defined(_WIN32) + constexpr bool RunningOnWindows = true; +#else + constexpr bool RunningOnWindows = false; +#endif OS << '"'; for (const auto c : Arg) { - if (c == '"' || c == '\\' || c == '$') + if (c == '"' || (c == '\\' && !RunningOnWindows) || c == '$') OS << '\\'; OS << c; }
Index: clang/test/Driver/backslash.c =================================================================== --- /dev/null +++ clang/test/Driver/backslash.c @@ -0,0 +1,6 @@ +// RUN: %clang -fsyntax-only %s -### 2>&1 | FileCheck %s +// Ensure the treatment of backslashes is consistent in the printed InstalledDir +// and commands. We would previous escape backslashes in the printed commands +// but not in the printed InstallDir, causing the match to fail. +// CHECK: InstalledDir: [[INSTALLEDDIR:.+$]] +// CHECK: "[[INSTALLEDDIR]]{{[/\\]}}clang Index: clang/lib/Driver/Job.cpp =================================================================== --- clang/lib/Driver/Job.cpp +++ clang/lib/Driver/Job.cpp @@ -107,9 +107,26 @@ } // Quote and escape. This isn't really complete, but good enough. + // On Windows, the backslash is the path separator, and escaping the backslash + // results in unnatural-looking output that's also inconsistent with how paths + // are printed out elsewhere (e.g. when printing out the installation + // directory). Windows native shells (cmd and PowerShell) use different escape + // characters so escaping the backslash is unnecessary. bash (the most common + // non-native shell on Windows) uses backslash as an escape character inside + // double quotes only when it's followed by $, `, ", \, or a literal newline; + // the last three are disallowed in paths on Windows and the first two are + // unlikely, so this shouldn't cause issues in practice. Note that we always + // enclose an argument containing backslashes in quotes even if we aren't + // escaping the backslashes, since bash always interprets it as an escape + // character outside quotes. +#if defined(_WIN32) + constexpr bool RunningOnWindows = true; +#else + constexpr bool RunningOnWindows = false; +#endif OS << '"'; for (const auto c : Arg) { - if (c == '"' || c == '\\' || c == '$') + if (c == '"' || (c == '\\' && !RunningOnWindows) || c == '$') OS << '\\'; OS << c; }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits