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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

Thanks for the review!

In D111879#3095373 <https://reviews.llvm.org/D111879#3095373>, @dexonsmith wrote:

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

Thanks! Yeah D112289 <https://reviews.llvm.org/D112289> covers most of the refactorings needed. I'll rebase this on top of both of your patches, and split out the remaining refactorings too (which can go in either on their own, or squashed into D112289 <https://reviews.llvm.org/D112289>, I don't have a preference).



================
Comment at: llvm/include/llvm/Support/Path.h:28
 
-enum class Style { windows, posix, native };
+enum class Style { windows, windows_forward, posix, native };
 
----------------
dexonsmith wrote:
> 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.
Thanks, that sounds both clear and intuitive. I'll reorder `native` to the front too.


================
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);
----------------
dexonsmith wrote:
> 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()`?
The name is indeed a bit vague, but I tried to align it with https://en.cppreference.com/w/cpp/filesystem/path/make_preferred which does the same - but if you prefer clarity over alignment with that, I don't mind changing  - `make_preferred_separators` is quite clear.

Currently, this is implemented on top of `native`, which in addition to changing separators also expands `~` into the home directory path on Windows. I wonder if it would be better to keep `make_preferred` (or whatever we settle on) doing only the separator adjustment and nothing else. Although I don't think the distinction would matter in practice anyway.

Regarding dropping this and pushing the condition to the caller - actually the vast majority of callers of this are in llvm/lib/Support/Windows (most of them added in D111880), so for them, the condition isn't really needed. But in my full patchset, I do have a couple calls elsewhere scattered around the llvm and clang subdirs, and the intent is that you can have a fairly concise call to `make_preferred` in such codepaths if needed without taking up mental and visual space with the full condition there.

I rewrote the doxyen comment into this:
```
/// For Windows path styles, convert path to use the preferred path separators. 
/// For other styles, do nothing.
///
/// @param path A path that is transformed to preferred format.
```



================
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));
----------------
dexonsmith wrote:
> 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.
I don't have a strong preference for `std::replace_if`, a plain loop is probably just as good.


================
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);
+}
----------------
dexonsmith wrote:
> 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).
Sure, that sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111879