Page MenuHomePhabricator

[Windows] Separate elements in -print-search-dirs with semicolons
ClosedPublic

Authored by mstorsjo on Apr 25 2019, 2:55 AM.

Details

Summary

Path lists on windows should always be separated by semicolons, not colons. Reuse llvm::sys::EnvPathSeparator for this purpose (as that's also a path list that is separated in the same way).

Alternatively, this could just be a local ifdef _WIN32 in this function, or should we generalize the existing EnvPathSeparator to e.g. a llvm::sys::path::PathListSeparator?

Does this change need a test? I guess we could add a separate file with "REQUIRES: system-windows" and check if the output does contain a semicolon, but I'm not sure how meaningful that is, especially as the default list printed only contains one single path element.

This was noted within Qt (and worked around with a creative regex) at https://codereview.qt-project.org/247331.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Apr 25 2019, 2:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2019, 2:55 AM

I feel something like llvm::sys::path::PathListSeparator would be nicer, since AFAIK any path list should be separated with semicolons on Windows and colons everywhere else.

rnk added a comment.Apr 25 2019, 10:32 AM

Alternatively, this could just be a local ifdef _WIN32 in this function, or should we generalize the existing EnvPathSeparator to e.g. a llvm::sys::path::PathListSeparator?

I think it's clear as is. We're just reusing the environment path separator, which is used for INCLUDE, LIBS, PATH, and other path list environment variables.

Does this change need a test?

If we don't already have tests for the colon separator, I don't think we need new tests to verify we get : on posix and ; on Windows.

rnk accepted this revision.Apr 25 2019, 10:34 AM

I feel something like llvm::sys::path::PathListSeparator would be nicer, since AFAIK any path list should be separated with semicolons on Windows and colons everywhere else.

I think the dominant use case for EnvPathSeparator is separating a list of paths in environment variables. We have other use cases for it in lists of paths that aren't environment variables (-print-search-dirs), but renaming it just seems like a minor improvement, which seems like it should be separate from this change.

This revision is now accepted and ready to land.Apr 25 2019, 10:34 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2019, 1:02 PM