[PATCH] D89463: [yaml2obj][ELF] - Simplify the code that performs sections validation.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Just seen you committed it whilst I was mid review. My comments still stand :-)



================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:196-197
 
+  // Some derived sections might have own special entries. This method returns
+  // vector of <entry name, is used> pairs. It is used for sections validation.
+  virtual std::vector<std::pair<StringRef, bool>> getEntries() const {
----------------



================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:198
+  // vector of <entry name, is used> pairs. It is used for sections validation.
+  virtual std::vector<std::pair<StringRef, bool>> getEntries() const {
+    return {};
----------------
Maybe rather than a vector of pairs, a `StringMap<bool>` might be more straightforward. Not sure if order is an issue though.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:245
+  std::vector<std::pair<StringRef, bool>> getEntries() const override {
+    return {{"Entries", !!Entries}};
+  };
----------------
Seems more easily understandable to me. Same throughout.


================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:311
     return "Entries and Content can't be used together";
-  return StringRef();
+  return "";
 }
----------------
MaskRay wrote:
> Seems inconsistent with `{}` you use below..
FWIW, I'd prefer `""` throuhgout since it's more readable (you don't have to refer to the return type to figure out that it's a string-like thing), but don't feel strongly about it.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1439
+    std::string Msg;
+    for (size_t I = 0; I < EntV.size(); ++I) {
+      StringRef Name = EntV[I].first;
----------------
Traditional LLVM style says to precalculate the size, rather than to do it on each iteration of the loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89463