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

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

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, with one more nitpick.



================
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);
----------------
mstorsjo wrote:
> 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.
> ```
> 
New docs look great. I missed `std::make_preferred()` -- better just to use the same name here, so let's keep the name as you suggest.


================
Comment at: llvm/lib/Support/Path.cpp:39-46
   inline Style real_style(Style style) {
-    if (is_style_posix(style))
-      return Style::posix;
-    return Style::windows;
+    if (style == Style::native) {
+      if (is_style_posix(style))
+        return Style::posix;
+      return Style::windows;
+    }
+    return style;
----------------
I suggest inverting the new check with an early `return` to avoid unnecessarily adding nesting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111879