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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/TAPI/ELFInterfaceFile.h:69
+
+  static const uint32_t SUPPORTED_FILE_TYPES =
+      FileType::TBE_V1 | FileType::ELF_32LE | FileType::ELF_32BE |
----------------
You're mixing uint32_t and unsigned. Whilst in practice these will likely be the same, it probably makes sense to be consistent (i.e. the type of SUPPORTED_FILE_TYPES should match that of the FileType enum). You could, though it might be unnecessarily verbose, use decltype(std::underlying_type<FileType>) here, if you wanted to guarantee that the types match.


================
Comment at: llvm/include/llvm/TAPI/ELFInterfaceFile.h:77
+  ELFStub Stub;
+  enum FileType Type;
+};
----------------
Why the explicit "enum"? Isn't it unnecessary?


================
Comment at: llvm/include/llvm/TAPI/TBEHandler.h:17-20
+#include "llvm/Support/Error.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/YAMLTraits.h"
+#include "llvm/TAPI/ELFStubFile.h"
----------------
amontanez wrote:
> jhenderson wrote:
> > Most (all?) of these includes can be replaced with forward-declarations.
> It appears that ELFStubFile is the only one that can't be replaced since FileType is an enum.
I think you can forward declare `ELFInterfaceFile` in this file, to remove the dependency on that header here.


================
Comment at: llvm/lib/TAPI/ELFInterfaceFile.cpp:1
+//===- ELFInterfaceFile.cpp -------------------------------------*- C++ -*-===//
+//
----------------
Not looked at common practice, but I noticed that according to the docs "*- C++ -*" is unnecessary in .cpp files.


================
Comment at: llvm/lib/TAPI/ELFInterfaceFile.cpp:25
+    ErrorMsg.append("`\n");
+    return createStringError(std::make_error_code(std::errc::invalid_argument),
+                             ErrorMsg.c_str());
----------------
If you use `llvm::errc`, you don't need the `std::make_error_code` here.


================
Comment at: llvm/lib/TAPI/TBEHandler.cpp:74
+    if (Symbol.Type == ELFSymbolType::NoType) {
+      IO.mapOptional("Size", Symbol.Size, (uint64_t)0u);
+    } else if (Symbol.Type == ELFSymbolType::Func) {
----------------
`(uint64_t)0u` looks weird to me. Do you need to be explicit about the type here?


================
Comment at: llvm/lib/TAPI/TBEHandler.cpp:115
+    IO.mapRequired("SoName", Stub.SoName);
+    IO.mapRequired("Arch", (ELFArchMapper &)Stub.Arch);
+    IO.mapOptional("Symbols", Stub.Symbols);
----------------
I'm guessing this is some magic in the YAML code? I don't really understand it, if I'm honest.


================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:37-40
+    // This forces null terminated strings.
+    std::string LeftLine = Line1.str();
+    std::string RightLine = Line2.str();
+    EXPECT_STREQ(LeftLine.data(), RightLine.data());
----------------
amontanez wrote:
> jhenderson wrote:
> > Dumb question maybe, but why can't you do EXPECT_EQ(Line1, Line2)?
> I tried that and it is functionally correct. Unfortunately, the output when a check fails is difficult to read.
Okay, makes sense.

Aside: I reckon StringRef should be null terminated - it's a reference to a string, either a C-style literal one (complete with '\0') or a std::string (which is null-terminated in modern C++).


================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:42-44
+    std::string LeftLine = Line1.str();
+    std::string RightLine = Line2.str();
+    EXPECT_STREQ(LeftLine.data(), RightLine.data());
----------------
Maybe to make this look slightly less hideous this should be compressed into one line:
`EXPECT_STREQ(Line1.str().data(), Line2.str().data())`


================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:118
+TEST(LLVMTAPI, YAMLReadsNoSyms) {
+  const unsigned int NUM_SYMS = 0;
+  const char Data[] = "--- !tapi-tbe-v1\n"
----------------
Move this inline.


Repository:
  rL LLVM

https://reviews.llvm.org/D53051