Make FormatStyle.GetStyleOfFile test work on MSVC

[amaiorano]

Summary

This is more a workaround than a real fix. The real problem is that FS.addFile uses clang::vfs::FileSystem::makeAbsolute to convert the input path to an absolute path, while getStyle uses llvm::sys::fs::make_absolute, which while named similarly, behave differently. Both will join the input path to the current working directory (CWD), except that in the former case, you need to have set the CWD explicitly via clang::vfs::FileSystem::setCurrentWorkingDirectory, while the latter retrieves the CWD from the platform abstraction (llvm::sys::fs::current_path).

A better fix might be to make clang::vfs::FileSystem match the behaviour of llvm::sys::fs, having it retrieve the CWD from the platform, rather than having the client set it explicitly. Or perhaps clang::vfs::FileSystem should be rewritten in terms of llvm::sys::fs, and deprecate the former so that code moves towards using the latter.

[klimek]

Why isn't the right solution to make getStyle() use vfs::FileSystem? Generally, everything in clang-format (well, in clang) should use vfs::FileSystem for file access.

[amaiorano]

In D27971#627543, @klimek wrote:

Why isn't the right solution to make getStyle() use vfs::FileSystem? Generally, everything in clang-format (well, in clang) should use vfs::FileSystem for file access.

You're absolutely right, this is the right solution. I was under the false impression that llvm::fs was preferred. What threw me off was that vfs::FileSystem::status() returns llvm::fs::file_type::* values.

I already made the change locally to getStyle() to use vfs::FileSystem and it works great. Will update the patch asap. Thanks!

[amaiorano]

As agreed, modified getStyle to make use of vfs::FileSystem::makeAbsolute. I would modify the body of the commit as follows:

Modify getStyle to use vfs::FileSystem::makeAbsolute just like FS.addFile does,
rather than sys::fs::make_absolute. The latter gets the CWD from the platform,
while the former expects it to be set by the client, causing a mismatch when
converting relative paths to absolute.

(not sure if I should have just modified the Summary section of this Phabricator review)

[klimek]

Apart from the error handling LG. Thanks!

lib/Format/Format.cpp

1920–1922 ↗

(On Diff #82185)

I think if makeAbsolute doesn't work, we will probably want to err out here:
if (EC) {

llvm::errs() << ... << EC.message << ...
return Style;

}

[amaiorano]

lib/Format/Format.cpp

1920–1922 ↗

(On Diff #82185)

Okay. I based myself on the fact that the old code didn't do error checking on fs::make_absolute - in fact most calls to fs::make_absolute in the codebase assert on the result. I suppose it's because it's just a string manipulation function, and if it fails, the subsequent code that will make use of the path will fail with a more useful error.

In any case, I don't mind handling the error here as you propose.

[klimek]

lib/Format/Format.cpp

1920–1922 ↗

(On Diff #82185)

I don't think it's overly important in this case. Generally, I like a good error message more than an assert that gives me no info on what happened...

cfe/trunk/lib/Format/Format.cpp

if (!getPredefinedStyle(StyleName, Style.Language, &Style))
       llvm::errs() << "Invalid value for -style, using " << FallbackStyle
                    << " style\n";
     return Style;
   }
 
   // Look for .clang-format/_clang-format file in the file's parent directories.
   SmallString<128> UnsuitableConfigFiles;
   SmallString<128> Path(FileName);
-  llvm::sys::fs::make_absolute(Path);
+  if (std::error_code EC = FS->makeAbsolute(Path)) {
+    llvm::errs() << EC.message() << "\n";
+    return Style;
+  }
+
   for (StringRef Directory = Path; !Directory.empty();
        Directory = llvm::sys::path::parent_path(Directory)) {
 
     auto Status = FS->status(Directory);
     if (!Status ||
         Status->getType() != llvm::sys::fs::file_type::directory_file) {
       continue;
     }

cfe/trunk/unittests/Format/FormatTest.cpp

EXPECT_EQ("auto a = unique_ptr<Foo<Bar>[10]>;",
             format("auto a = unique_ptr < Foo < Bar>[ 10]> ;"));
 
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInSquareBrackets = true;
   EXPECT_EQ("auto a = unique_ptr<Foo<Bar>[ 10 ]>;",
             format("auto a = unique_ptr < Foo < Bar>[10]> ;", Spaces));
 }
 
-// Since this test case uses UNIX-style file path. We disable it for MS
-// compiler.
-#if !defined(_MSC_VER) && !defined(__MINGW32__)
-
 TEST(FormatStyle, GetStyleOfFile) {
   vfs::InMemoryFileSystem FS;
   // Test 1: format file in the same directory.
   ASSERT_TRUE(
       FS.addFile("/a/.clang-format", 0,
                  llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM")));
   ASSERT_TRUE(
       FS.addFile("/a/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
ASSERT_TRUE(
       FS.addFile("/c/.clang-format", 0,
                  llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
   ASSERT_TRUE(FS.addFile("/c/sub/sub/sub/test.cpp", 0,
                          llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style3 = getStyle("file", "/c/sub/sub/sub/test.cpp", "LLVM", "", &FS);
   ASSERT_EQ(Style3, getGoogleStyle());
 }
 
-#endif // _MSC_VER
-
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
   // Column limit is 20.
   std::string Code = "Type *a =\n"
                      "    new Type();\n"
                      "g(iiiii, 0, jjjjj,\n"
                      "  0, kkkkk, 0, mm);\n"
                      "int  bad     = format   ;";
   std::string Expected = "auto a = new Type();\n"