Support: Expose sys::path::is_style_{posix,windows,native}()

[dexonsmith]

Summary

Expose three helpers in namespace llvm::sys::path to detect the
path rules followed by sys::path::Style.

• is_style_posix()
• is_style_windows()
• is_style_native()

This are constexpr functions that that will allow a bunch of
path-related code to stop checking _WIN32.

Originally I looked at adding system_style(), analogous to
sys::endian::system_endianness(), but future patches (from others) will
add more Windows style variants for slash preferences. These helpers
should be resilient to that change, allowing callers to detect basic
path rules.

[dexonsmith]

I posted a patch with a bit more adoption at https://reviews.llvm.org/D112289.

[Bigcheese]

Looks good. We shouldn't be checking _WIN32 just for path style questions.

[mstorsjo]

llvm/include/llvm/Support/Path.h

37

FYI, I have a patchset for extending the handling of windows path styles into two styles, windows-with-backslashes and windows-with-forward-slashes; the first patch is up for review at https://reviews.llvm.org/D111879, but later in the series, I would be extending system_style() to return either of the windows styles in various cases.

My current patch that does that (that I haven't sent for review yet) is at https://github.com/mstorsjo/llvm-project/commit/c609500ef26145dcca7d853c9ceabbe1402fc4f2, where I just add another #ifdef __MINGW32__ here, which would work fine with constexpr.

But I'm considering other alternatives for enabling that mode, where having the function as constexpr in a header isn't entirely ideal. One alternative would be to make a cmake option for choosing the default preferred path form - and having a generated config.h included in a pretty widely included header wouldn't be good. An even wilder option (that I don't think I'd propose though) would be to make it runtime settable (as a global variable/setting).

I'd like if @rnk could chime in here - what amount of flexibility/settability do you forsee for that feature - does that conflict with having it constexpr in a header?

Then on the other hand, even if it is constexpr in a header right now, I guess it should be doable to change it to a non-constexpr function later if needed, as long as it isn't used in a constexpr context in the callers. It won't generate quite as optimal code in the callers, but I think the difference would be negligible.

[mstorsjo]

llvm/include/llvm/Support/Path.h

37

Also, in D111879 I'm adding a bool is_style_windows(Style style) helper inside Path.cpp, but I guess it could be exposed publicly too. And with that in mind, another way of keeping the constexpr-ness, but much less elegant though, is to add a separate bool system_style_is_windows() - because regardless which slash preference is chosen on windows, we still know for sure at compile time that it's going to be either of the windows path styles.

[rnk]

llvm/include/llvm/Support/Path.h

37

Right, yes, thanks for that. I was thinking the effective slash style should be a runtime option, not a build option. I've learned that, in practice, Windows developers disagree on what path style they want from their tools. I would really like to be able to respond to every user issue about wrong slash direction by referring people to the relevant flag. So, either this function will not be constexpr for long, or it will not examine the runtime flag.

[dexonsmith]

llvm/include/llvm/Support/Path.h

37

How about this?

enum class Style {
  native,
  posix,
  windows_slash,     // added in your patch
  windows_backslash, // added in your patch
  windows = windows_backslash,
};

namespace detail {
constexpr bool is_system_style_windows() {
#if defined(_WIN32)
  return true;
#else
  return false:
#endif
}
} // end namespace detail

constexpr bool is_style_posix(Style S) {
  return S == Style::native ? !detail::is_system_style_windows() : S == Style::posix;
}

constexpr bool is_style_windows(Style S) { return !is_style_posix(); }

constexpr bool is_style_native(Style S) {
  return is_style_windows(S) == detail::is_system_style_windows();
}

All the callers I have in mind can happily use is_style_posix(), is_style_windows(), or is_style_native().

[dexonsmith]

Updated description and patch to follow the suggestion in my last comment.

[mstorsjo]

llvm/include/llvm/Support/Path.h

37

Thanks! This looks great to me - keeping it (fairly) straightforward for callers while still flexible enough for the upcoming changes. I'll rebase my changes on top of this.

[mstorsjo]

LGTM, thanks! This seems to work great with my patches on top.

llvm/include/llvm/Support/Path.h

 #include <system_error>
 
 namespace llvm {
 namespace sys {
 namespace path {
 
 enum class Style { windows, posix, native };
 
+/// Check if \p S uses POSIX path rules.
+constexpr bool is_style_posix(Style S) {
+  if (S == Style::posix)
+    return true;
+  if (S != Style::native)
+    return false;
+#if defined(_WIN32)
+  return false;
+#else
+  return true;
+#endif
+}
+
+/// Check if \p S uses Windows path rules.
+constexpr bool is_style_windows(Style S) { return !is_style_posix(S); }
+
+/// Check if \p S uses the same path rules as Style::native.
+constexpr bool is_style_native(Style S) {
+  return is_style_posix(S) == is_style_posix(Style::native);
+}
+
 /// @name Lexical Component Iterator
 /// @{
 
 /// Path iterator.
 ///
 /// This is an input iterator that iterates over the individual components in
 /// \a path. The traversal order is as follows:
 /// * The root-name element, if present.

llvm/lib/Support/Path.cpp

 using namespace llvm::support::endian;
 
 namespace {
   using llvm::StringRef;
   using llvm::sys::path::is_separator;
   using llvm::sys::path::Style;
 
   inline Style real_style(Style style) {
-#ifdef _WIN32
-    return (style == Style::posix) ? Style::posix : Style::windows;
-#else
-    return (style == Style::windows) ? Style::windows : Style::posix;
-#endif
+    if (is_style_posix(style))
+      return Style::posix;
+    return Style::windows;
   }
 
   inline const char *separators(Style style) {
     if (real_style(style) == Style::windows)
       return "\\/";
     return "/";
   }
 

llvm/unittests/Support/Path.cpp

 namespace {
 
 struct FileDescriptorCloser {
   explicit FileDescriptorCloser(int FD) : FD(FD) {}
   ~FileDescriptorCloser() { ::close(FD); }
   int FD;
 };
 
+TEST(is_style_Style, Works) {
+  using namespace llvm::sys::path;
+  // Check platform-independent results.
+  EXPECT_TRUE(is_style_posix(Style::posix));
+  EXPECT_TRUE(is_style_windows(Style::windows));
+  EXPECT_FALSE(is_style_posix(Style::windows));
+  EXPECT_FALSE(is_style_windows(Style::posix));
+
+  // Check platform-dependent results.
+#if defined(_WIN32)
+  EXPECT_FALSE(is_style_posix(Style::native));
+  EXPECT_TRUE(is_style_windows(Style::native));
+#else
+  EXPECT_TRUE(is_style_posix(Style::native));
+  EXPECT_FALSE(is_style_windows(Style::native));
+#endif
+
+  // 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));
+}
+
 TEST(is_separator, Works) {
   EXPECT_TRUE(path::is_separator('/'));
   EXPECT_FALSE(path::is_separator('\0'));
   EXPECT_FALSE(path::is_separator('-'));
   EXPECT_FALSE(path::is_separator(' '));
 
   EXPECT_TRUE(path::is_separator('\\', path::Style::windows));
   EXPECT_FALSE(path::is_separator('\\', path::Style::posix));
 
-#ifdef _WIN32
-  EXPECT_TRUE(path::is_separator('\\'));
-#else
-  EXPECT_FALSE(path::is_separator('\\'));
-#endif
+  EXPECT_EQ(path::is_style_windows(path::Style::native),
+            path::is_separator('\\'));
 }
 
 TEST(is_absolute_gnu, Works) {
   // Test tuple <Path, ExpectedPosixValue, ExpectedWindowsValue>.
   const std::tuple<StringRef, bool, bool> Paths[] = {
       std::make_tuple("", false, false),
       std::make_tuple("/", true, true),
       std::make_tuple("/foo", true, true),
       std::make_tuple("\\", false, true),
       std::make_tuple("\\foo", false, true),
       std::make_tuple("foo", false, false),
       std::make_tuple("c", false, false),
       std::make_tuple("c:", false, true),
       std::make_tuple("c:\\", false, true),
       std::make_tuple("!:", false, true),
       std::make_tuple("xx:", false, false),
       std::make_tuple("c:abc\\", false, true),
       std::make_tuple(":", false, false)};
 
   for (const auto &Path : Paths) {
     EXPECT_EQ(path::is_absolute_gnu(std::get<0>(Path), path::Style::posix),
               std::get<1>(Path));
     EXPECT_EQ(path::is_absolute_gnu(std::get<0>(Path), path::Style::windows),
               std::get<2>(Path));
+
+    constexpr int Native = is_style_posix(path::Style::native) ? 1 : 2;
+    EXPECT_EQ(path::is_absolute_gnu(std::get<0>(Path), path::Style::native),
+              std::get<Native>(Path));
   }
 }
 
 TEST(Support, Path) {
   SmallVector<StringRef, 40> paths;
   paths.push_back("");
   paths.push_back(".");
   paths.push_back("..");