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

[mstorsjo]

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.

[smeenai]

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]

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]

In D61121#1479031, @smeenai wrote:

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.

cfe/trunk/lib/Driver/Driver.cpp

if (C.getArgs().hasArg(options::OPT_print_resource_dir)) {
     return false;
   }
 
   if (C.getArgs().hasArg(options::OPT_print_search_dirs)) {
     llvm::outs() << "programs: =";
     bool separator = false;
     for (const std::string &Path : TC.getProgramPaths()) {
       if (separator)
-        llvm::outs() << ':';
+        llvm::outs() << llvm::sys::EnvPathSeparator;
       llvm::outs() << Path;
       separator = true;
     }
     llvm::outs() << "\n";
     llvm::outs() << "libraries: =" << ResourceDir;
 
     StringRef sysroot = C.getSysRoot();
 
     for (const std::string &Path : TC.getFilePaths()) {
       // Always print a separator. ResourceDir was the first item shown.
-      llvm::outs() << ':';
+      llvm::outs() << llvm::sys::EnvPathSeparator;
       // Interpretation of leading '=' is needed only for NetBSD.
       if (Path[0] == '=')
         llvm::outs() << sysroot << Path.substr(1);
       else
         llvm::outs() << Path;
     }
     llvm::outs() << "\n";
     return false;