[PATCH] D125800: [COFF] Add vfsoverlay flag

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Seems good to me in principle, I just have a few nits.

Have you checked whether this affects the performance when linking something large-ish without the vfs overlay?



================
Comment at: lld/COFF/Driver.cpp:453
     sys::path::append(path, filename);
+    path = SmallString<128>{getFilename(path.str())};
     if (sys::fs::exists(path.str()))
----------------
Is the explicit SmallString constructor call needed, or would just `path = getFilename(...)` work?


================
Comment at: lld/COFF/Driver.cpp:458
       path.append(".obj");
+      path = SmallString<128>{getFilename(path.str())};
       if (sys::fs::exists(path.str()))
----------------
Ditto.


================
Comment at: lld/COFF/Driver.cpp:1367
+  if (!arg)
+    return {};
+
----------------
nit: I think `return nullptr` would be more common llvm style.


================
Comment at: lld/COFF/Driver.cpp:1371
+  if (!bufOrErr)
+    return {};
+
----------------
Shouldn't we give the user an error message?


================
Comment at: lld/COFF/Driver.cpp:1374
+  return vfs::getVFSFromYAML(std::move(*bufOrErr), /*DiagHandler*/ nullptr,
+                             arg->getValue());
+}
----------------
Ditto if this fails.


================
Comment at: lld/test/COFF/vfsoverlay.test:6
+
+# RUN: lld-link %S/Inputs/hello64.obj /libpath:/noexist /out:%t.exe /entry:main /defaultlib:notstd64 /vfsoverlay:%t/overlay.yaml
+
----------------
Can we have tests cases for when the `/vfsoverlay` file doesn't exist or is invalid?


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

https://reviews.llvm.org/D125800