This is an archive of the discontinued LLVM Phabricator instance.

Make FormatStyle.GetStyleOfFile test work on MSVC
ClosedPublic

Authored by amaiorano on Dec 19 2016, 9:39 PM.

Details

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.

Diff Detail

Event Timeline

amaiorano updated this revision to Diff 82063.Dec 19 2016, 9:39 PM
amaiorano retitled this revision from to Make FormatStyle.GetStyleOfFile test work on MSVC .
amaiorano updated this object.
amaiorano added reviewers: klimek, djasper, hans, cfe-commits.
klimek edited edge metadata.Dec 20 2016, 1:10 AM

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.

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 updated this revision to Diff 82185.Dec 20 2016, 5:46 PM
amaiorano edited edge metadata.

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 accepted this revision.Dec 21 2016, 12:24 AM
klimek edited edge metadata.

Apart from the error handling LG. Thanks!

lib/Format/Format.cpp
1920–1922

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

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

}

This revision is now accepted and ready to land.Dec 21 2016, 12:24 AM
amaiorano added inline comments.Dec 21 2016, 3:44 AM
lib/Format/Format.cpp
1920–1922

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 added inline comments.Dec 21 2016, 4:16 AM
lib/Format/Format.cpp
1920–1922

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...

This revision was automatically updated to reflect the committed changes.