[PATCH] D69648: Add VFS support for sanitizers' blacklist' 2

[Ilya Biryukov via Phabricator via llvm-commits]

ilya-biryukov added inline comments.


================
Comment at: clang/include/clang/Basic/SanitizerSpecialCaseList.h:18
 #include "clang/Basic/Sanitizers.h"
+#include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/StringRef.h"
----------------
NIT: `#include "llvm/Support/VirtualFileSystem.h"` should be enough


================
Comment at: clang/lib/Basic/SanitizerSpecialCaseList.cpp:24
+
+  for (const auto &Path : Paths) {
+    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MaybeBuffer =
----------------
Why not pass `VFS` to the `SpecialCaseList::createInternal` instead?
We're duplicating the code of `createInternal` here.


================
Comment at: clang/lib/Basic/SanitizerSpecialCaseList.cpp:25
+  for (const auto &Path : Paths) {
+    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MaybeBuffer =
+        VFS.getBufferForFile(Path);
----------------
NIT: maybe use `auto`. The type is quite long, spelling it out seems to hurt readability


================
Comment at: clang/lib/Basic/SanitizerSpecialCaseList.cpp:27
+        VFS.getBufferForFile(Path);
+    if (std::error_code EC = MaybeBuffer.getError()) {
+      Error = (Twine("can't open file '") + Path + "': " + EC.message()).str();
----------------
NIT: it's probably more idiomatic to use a boolean conversion here:
```
if (!MaybeBuffer) {
  Error = (Twine("can't open file '") + Path + "': " + MaybeBuffer.getError().message()).str();
  return;
}
```

But up to you, obviously


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69648/new/

https://reviews.llvm.org/D69648