[PATCH] D70378: [LLD][COFF] Cover usage of LLD as a library

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/test/ELF/cgprofile-shared-warn.s:12
+# RUN: ld.lld --gc-sections %t.o %t1.so -o /dev/null 2>&1 | FileCheck %s
+# CHECK-NOT: ld.lld: error:
 
----------------
No need to include "ld.lld" to the check line.


================
Comment at: lld/test/ELF/zdefs.s:14
+# RUN: ld.lld -z defs -z undefs -shared %t.o -o /dev/null 2>&1 | FileCheck -check-prefix=ERR2 %s
+# ERR2-NOT: error: undefined symbol
 
----------------
I think testing just "error: " is enough. You do not expect any errors here, not just "undefined symbol" error.
Also, it is safer to provide a minimal check string when `-NOT` is used. In case if we change "undefined symbol"
message, it might be easy to forget to update such check lines.


================
Comment at: lld/tools/lld/lld.cpp:133
 
+static unsigned inTestVerbosity() {
+  unsigned v{};
----------------
It needs a comment to explain what does it return.


================
Comment at: lld/tools/lld/lld.cpp:178
+      SaveAndRestore<bool> sr(errorHandler().verbose, true);
+      log("LLD_IN_TEST (new LLD session)");
+    }
----------------
May be I am missing something, but I read it as: you use `SaveAndRestore` to set
`errorHandler().verbose` to true and restore the value back, because want to bypass
the check in:

```
void ErrorHandler::log(const Twine &msg) {
  if (!verbose)
    return;
  std::lock_guard<std::mutex> lock(mu);
  lld::errs() << logName << ": " << msg << "\n";
}
```

My question: why instead not to use `lld::errs()` directly here?
E.g:

```
lld::errs() << ErrorHandler::logName <<  ": "LLD_IN_TEST (new LLD session)\n";
```


================
Comment at: lld/tools/lld/lld.cpp:184
+    else
+      assert(r == *ret);
+    cl::ResetAllOptionOccurrences();
----------------
Should we `return r;` here when `*ret != r`? This should make a test fail even when asserts are disabled I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70378