This is an archive of the discontinued LLVM Phabricator instance.

Lift VFS from clang to llvm
ClosedPublic

Authored by JDevlieghere on Oct 2 2018, 7:57 AM.

Details

Summary

This patch moves the virtual file system form clang to llvm so it can be used by more projects.

Concretely the patch:

  • Moves VirtualFileSystem.{h|cpp} from clang/Basic to llvm/Support.
  • Moves the corresponding unit test from clang to llvm.
  • Moves the vfs namespace from clang::vfs to llvm::vfs.
  • Formats the lines affected by this change, mostly this is the result of the added llvm namespace.

RFC on the mailing list: http://lists.llvm.org/pipermail/llvm-dev/2018-October/126657.html

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Oct 2 2018, 7:57 AM
MaskRay added a subscriber: MaskRay.Oct 2 2018, 2:18 PM
chapuni added a subscriber: chapuni.Oct 2 2018, 2:53 PM
t-tye added a subscriber: t-tye.Oct 2 2018, 7:55 PM
grimar added a subscriber: grimar.EditedOct 3 2018, 2:10 AM

Seems there are a lot of changes caused by clang-formatting. I suggest to not mix the cosmetic and functionality changes and either revert them (remove from this patch) or commit as NFC separately at first.

Seems there are a lot of changes caused by clang-formatting. I suggest to not mix the cosmetic and functionality changes and either revert them (remove from this patch) or commit as NFC separately at first.

Thanks for having a look, George! I ran clang-format on the change set, so only lines that were touched were reformatted. In some cases formatting a line causes the surrounding lines to be changed, but that's to be expected (think 80-col violations). The three files that got moved got formatted as well, but since they're moved cross-repo the history was going to be affected anyway. If you see changes that are totally unrelated to the patch, please do let me know, I totally agree with you that they shouldn't be part of this patch.

JDevlieghere edited the summary of this revision. (Show Details)Oct 3 2018, 5:20 AM

Are you sure the diff is okay? I see only a move on VirtualFileSystem.cpp but I really think there were namespace clang { lines in it too.

clang-tools-extra/clang-tidy/ClangTidyOptions.h
222–225 ↗(On Diff #167951)

The formatting went sideways here...

  • Fix a few formatting issues.
JDevlieghere marked an inline comment as done.Oct 3 2018, 8:38 AM

Are you sure the diff is okay? I see only a move on VirtualFileSystem.cpp but I really think there were namespace clang { lines in it too.

It's possible that I missed some usages between ifdef's, but the current diffs compiles on my machine. For the namespace change, see line 45 in the original VirtualFileSystem.h.

clang-tools-extra/clang-tidy/ClangTidyOptions.h
222–225 ↗(On Diff #167951)

Thanks! Fixed it in the latest diff.

This revision is now accepted and ready to land.Oct 4 2018, 8:32 AM
whisperity accepted this revision.Oct 6 2018, 2:35 AM

Looks good. This change seems very beneficial, I have a little downstream project I'll have to commit an update for when this releases but it is a trivial change on the users' perspective.

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
cfe/trunk/unittests/Basic/FileManagerTest.cpp