This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dexonsmith on Oct 21 2021, 6:32 PM.



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.

Diff Detail

Event Timeline

dexonsmith created this revision.Oct 21 2021, 6:32 PM
dexonsmith requested review of this revision.Oct 21 2021, 6:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2021, 6:32 PM
Bigcheese accepted this revision.Oct 28 2021, 9:52 AM

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

This revision is now accepted and ready to land.Oct 28 2021, 9:52 AM
mstorsjo added inline comments.

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, 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, 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 added inline comments.Oct 28 2021, 2:39 PM

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 added inline comments.Oct 28 2021, 2:58 PM

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 added inline comments.Oct 28 2021, 3:41 PM

How about this?

enum class Style {
  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;
  return false:
} // 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 retitled this revision from Support: Expose sys::path::system_style() to Support: Expose sys::path::is_style_{posix,windows,native}().
dexonsmith edited the summary of this revision. (Show Details)
dexonsmith added reviewers: rnk, mstorsjo.

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

dexonsmith requested review of this revision.Oct 28 2021, 4:42 PM
mstorsjo added inline comments.Oct 29 2021, 12:04 AM

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 accepted this revision.Oct 29 2021, 3:26 AM

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

This revision is now accepted and ready to land.Oct 29 2021, 3:26 AM