[PATCH] D111879: [Support] Add a new path style for Windows with forward slashes

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

I have a few (mostly nitpicky) comments inline.

Also, please separate out the change to start using `is_windows_style()` into a separate/prep patch that's no functionality change (NFC). That'll make this diff smaller, and clarify where the behaviour is actually changing. (BTW, I updated https://reviews.llvm.org/D112288 to cover many of those before I looked here and saw all the conflicts; happy remove those if you want, but I had thought since my patch was NFC anyway I might as well clean them up.)



================
Comment at: llvm/include/llvm/Support/Path.h:28
 
-enum class Style { windows, posix, native };
+enum class Style { windows, windows_forward, posix, native };
 
----------------
I wonder if the names would be more clear as:
```
windows_slash,               // or windows_forward?
windows_backslash,           // or windows_backward?
windows = windows_backslash, // leave a "deprecated" comment, and eventually remove?
```
... and then prefer the explicit `windows_*` names, eventually removing the plain `windows`. This avoids guessing games. WDYT?

BTW, since you're shuffling this enum anyway, I wonder if you could reorder `native` to the front? That way zero-initializers can be used in the common case.


================
Comment at: llvm/include/llvm/Support/Path.h:244-249
+/// Convert path to the native form in place. On Windows, where both forward and
+/// backwards slashes are path separators, convert to the preferred form.
+/// On other platforms, this does nothing.
+///
+/// @param path A path that is transformed to preferred format.
+void make_preferred(SmallVectorImpl<char> &path, Style style = Style::native);
----------------
It took me a few reads to understand how this was different from `native`, I think because the first sentence is identical. Can you update that sentence to clarify?

Also, the name didn't make it obvious to me what it does. One idea I had for the name was `make_preferred_separators` (describing the feature of Windows that causes it to do something). Another was `native_if_windows` (clearly describing the behaviour).

Alternatively, after https://reviews.llvm.org/D112288 or similar, you could skip this entirely and have callers do the logic... is it reasonable for call sites to understand that this is triggered by `is_style_windows()`?


================
Comment at: llvm/lib/Support/Path.cpp:561
+    std::replace_if(Path.begin(), Path.end(),
+                    std::bind(is_separator, std::placeholders::_1, style),
+                    preferred_separator(style));
----------------
Personally I don't find replace_if easier to reason about than a simple `for` loop:
```
lang=c++
for (char &ch : Path)
  if (is_separator(ch, style))
    ch = preferred_separator(style);
```
but if you'd prefer `std::replace_if` then I'd suggest using a lambda, since I don't see other uses of `std::bind` in llvm/lib.


================
Comment at: llvm/lib/Support/Path.cpp:574-578
+void make_preferred(SmallVectorImpl<char> &path, Style style) {
+  if (!is_style_windows(style))
+    return;
+  native(path, style);
+}
----------------
Might be nice to write this inline in the header since it's so short, to document what it does. That'd require `is_style_windows()` to be available in the header (such as through https://reviews.llvm.org/D112288).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111879/new/

https://reviews.llvm.org/D111879