[PATCH] D112288: Support: Expose sys::path::system_style()

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

dexonsmith added inline comments.


================
Comment at: llvm/include/llvm/Support/Path.h:37
+  return Style::posix;
+#endif
+}
----------------
mstorsjo wrote:
> rnk wrote:
> > mstorsjo wrote:
> > > 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.
> > 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.
> 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.
How about this?
```
lang=c++
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()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112288