[PATCH] D53051: [llvm-tapi] initial commit, supports ELF text stubs

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've got a few more nits, but otherwise I'm happy with this. Somebody else should definitely take a look too though.



================
Comment at: llvm/include/llvm/TAPI/ELFInterfaceFile.h:59
+      return true;
+    else
+      return false;
----------------
Don't use else after an if that returns. See https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return.


================
Comment at: llvm/lib/TAPI/ELFInterfaceFile.cpp:22
+    return Error::success();
+  } else {
+    std::string ErrorMsg = "ELF incompatible with file type `";
----------------
Don't use else after an if that always returns.


================
Comment at: llvm/lib/TAPI/ELFInterfaceFile.cpp:23-26
+    std::string ErrorMsg = "ELF incompatible with file type `";
+    ErrorMsg.append(fileTypeToString(NewType));
+    ErrorMsg.append("`\n");
+    return createStringError(errc::invalid_argument, ErrorMsg.c_str());
----------------
createStringError is also overloaded to work with printf-style formatting, so you could do all of this simply in one line, with something like the following:

```
return createStringError(errc::invalid_argument,
        "ELF incompatible with file type `%s`\n",
        fileTypeToString(NewType).c_str());
```


================
Comment at: llvm/lib/TAPI/TBEHandler.cpp:58
+      return StringRef("Unknown architecture");
+    } else {
+      Value = ArchMap.lookup(Scalar);
----------------
Don't use else after return.


================
Comment at: llvm/lib/TAPI/TBEHandler.cpp:111
+  static void mapping(IO &IO, ELFStub &Stub) {
+    if (!IO.mapTag("!tapi-tbe-v1", true)) {
+      IO.setError("Invalid tbe version");
----------------
No need for the braces here.


================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:170
+
+  // Deliberately not in order to check that result is sorted
+  Stub.Symbols.push_back(SymNot);
----------------
Trailing full-stop needed on this comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D53051