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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked an inline comment as done.
mstorsjo added a comment.

Thanks! I noticed that the squashed in changes in 99023627010bbfefb71e25a2b4d056de1cbd354e <https://reviews.llvm.org/rG99023627010bbfefb71e25a2b4d056de1cbd354e> had mixed up with ones of `get_separator` and `separators` should use `is_style_windows`, but I’ll include the fix for that in this patch now. There’s no distinction between the two until this patch anyway.



================
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;
----------------
dexonsmith wrote:
> I suggest inverting the new check with an early `return` to avoid unnecessarily adding nesting.
Oh, yes - and that simplifies the diff even further.


================
Comment at: llvm/unittests/Support/Path.cpp:72
 
 TEST(is_style_Style, Works) {
   using namespace llvm::sys::path;
----------------
dexonsmith wrote:
> Also, might be good to add `windows_slash` and `windows_backslash` to the tests here.
Sure, will do. I also noticed that there’s no direct tests of `get_separator`, I’ll as that too.


================
Comment at: llvm/unittests/Support/Path.cpp:89-92
   // Check is_style_native().
   EXPECT_TRUE(is_style_native(Style::native));
   EXPECT_EQ(is_style_posix(Style::native), is_style_native(Style::posix));
   EXPECT_EQ(is_style_windows(Style::native), is_style_native(Style::windows));
----------------
dexonsmith wrote:
> FYI, I just caught myself wondering what `is_style_native()` *should* return for `Style::windows_slash` and `Style::windows_backslash` when running on Windows. In practice (at least right now) it will return true for both. But I'm not sure if most people looking at a call to `is_style_native()` could guess correctly, and I could see a justifications for being more fine-grained.
> 
> So, I removed it in 8077a19f66b50573a043ef1c405e2149e0c69cb7.
Good call, yes - I found that one a bit hard to see where it’d be used.


================
Comment at: llvm/unittests/Support/Path.cpp:1456
 
 #if defined(_WIN32)
   SmallString<64> PathHome;
----------------
dexonsmith wrote:
> Mostly unrelated to this patch, but I wonder if adding llvm::sys::fs::is_windows() (or is_system_windows or is_filesystem_windows or llvm::sys::is_windows or ...) and _posix() would lead to good cleanups.
Hmm, that might be nice in some cases, yeah, as long as the ifdefs don’t contain calls to windows-only functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111879