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

[Jan Korous via Phabricator via llvm-commits]

jkorous marked 4 inline comments as done.
jkorous added inline comments.


================
Comment at: clang/lib/Basic/SanitizerSpecialCaseList.cpp:24
+
+  for (const auto &Path : Paths) {
+    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MaybeBuffer =
----------------
ilya-biryukov wrote:
> Why not pass `VFS` to the `SpecialCaseList::createInternal` instead?
> We're duplicating the code of `createInternal` here.
I intended to keep the scope of this patch small but open to discussion. Seems like the only other place where `SpecialCaseList` is used is `XRayFunctionFilter`. The options I see here are:
1. Get rid of anything filesystem-related in `SpecialCaseList` and just require buffers in the interface.
2. Require `VFS` in `SpecialCaseList` interface and pass it from `XRayFunctionFilter` (seems like VFS would be used just in the constructor and not in the actual methods?).
3. Add `VFS` to only some method/s in `SpecialCaseList`. I'd consider this a regression in interface "quality".

I'd prefer #1 and unless someone can confirm that the XRay change is trivial I'd prefer to not do #2.


================
Comment at: clang/lib/Basic/SanitizerSpecialCaseList.cpp:25
+  for (const auto &Path : Paths) {
+    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MaybeBuffer =
+        VFS.getBufferForFile(Path);
----------------
ilya-biryukov wrote:
> NIT: maybe use `auto`. The type is quite long, spelling it out seems to hurt readability
I feel that spelling the type out here makes it a bit easier to understand what's going on later (`MaybeBuffer.get().get()`).


================
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();
----------------
ilya-biryukov wrote:
> 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
Thanks! I copied this from `SpecialCaseList`. Let's discuss eventual changes to `SpecialCaseList` interface first, I'd change this after.


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

https://reviews.llvm.org/D69648