[PATCH] D143693: [llvm-readobj] Add --memtag

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

You need to update llvm-readobj.rst as well with the new option (we have separate documents for the two of them because there are several options that aren't shared).



================
Comment at: llvm/test/tools/llvm-readobj/ELF/AArch64/memtag-missing.test:13
+# CHECK-NOT: Memtag Android Note
+# CHECK-NOT: Memtag Android Note
+
----------------
hctim wrote:
> jhenderson wrote:
> > There's no point in the duplicate CHECK-NOT. What are you trying to test with these lines?
> Removed the duplicate. We're trying to assert (unlike in `memtag.test`) that `readelf/readobj --memtag` indiscriminately prints "Memtag Dynamic Entries", but doesn't print a missing Android ELF note.
Is ordering important here? `CHECK-NOT` will just show that this pattern won't appear since the last positive match, so if the unexpected output appears before the "Memtag Dynamic Entries" bit, it'll not catch it. If ordering isn't relevant here, you may wish to use FileCheck's `"--implicit-check-not=Memtag Android Note"`.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2274
+        default:
+          return (Twine("Unknown (") + std::to_string(Value) + ")").str();
+      }
----------------
If memory serves me correctly, `Twine` has a constructor that effectively can do the `std::to_string`, so you should use that. Same goes for below.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5891
+  processNotesHelper(*this, /*StartNotesFn=*/PrintHeader,
+                     /*ProcessNoteFn=*/ProcessNote, /*FinishNotesFn=*/[]() {});
+}
----------------
Nit: has this been clang-formatted? If so, I'm marginally surprised that the format is `[]() {}` not `[](){}`.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5241
+    OS << "    < none found >\n";
+  for (const auto &DynamicEntry : DynamicEntries)
+    OS << "    " << DynamicEntry.first << ": " << DynamicEntry.second << "\n";
----------------
hctim wrote:
> jhenderson wrote:
> > Especially if you adopt my suggestion about adding a struct/class for the fields, you shouldn't use `auto` here in the loop (see Coding Standards about use of `auto` - it's not clear what the type is from the context).
> I've updated all the range-based loops to use `const auto& DynamicEntryKV` (note the new `KV` suffix). 
> 
> I agree with you (and the style guide) on "avoid `auto` for non-obvious type deductions", but IMHO using auto for a key-value iteration is idiomatic, as it's used for all `std::map<string, string>` iteration.
I guess in my mind, it wasn't obvious that this was a key-value situation, but the renaming helps, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143693