Propagate llvm::vfs::FileSystem from driver_env to Clang
#4537
Conversation
testing/file_test/file_test_base.h
Outdated
| @@ -77,7 +77,7 @@ class FileTestBase : public testing::Test { | |||
| // The return value should be an error if there was an abnormal error, and | |||
| // RunResult otherwise. | |||
| virtual auto Run(const llvm::SmallVector<llvm::StringRef>& test_args, | |||
| llvm::vfs::InMemoryFileSystem& fs, | |||
| llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> fs, | |||
There was a problem hiding this comment.
Can you elaborate why you're generally passing by value instead of by reference? That is, I understand a IntrusiveRefCntPtr is needed for ClangRunner. However, do we benefit from doing reference counting at each call boundary? (is incrementing the reference count for recursive calls the intended result?)
There was a problem hiding this comment.
I did not realize there was a performance concern here, and passing by value provides a simpler contract in general. I will make more of these references.
There was a problem hiding this comment.
Yes, these have complex construction/destruction so don't fall into the "trivial struct" case of copying.
| llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> fs = | ||
| new llvm::vfs::InMemoryFileSystem; |
There was a problem hiding this comment.
In some cases you use = initialization, in others you use () like:
llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> fs(
new llvm::vfs::InMemoryFileSystem);
What system are you using to choose which initialization style?
There was a problem hiding this comment.
Sorry, I was using () initialization for local variables, but that didn't work to default-initialize fields. I've now switched the () cases to =. There are a lot of choices for initialization in C++, have we settled on a convention?
There was a problem hiding this comment.
https://github.com/carbon-language/carbon-lang/blob/trunk/docs/project/cpp_style_guide.md#syntax-and-formatting, under "For initialization"
Note, I'm not sure there's a "best" choice here other than consistency.
As discussed in #4530 . This required switching to using
llvm::IntrusiveRefCntPtrin a number of places.