[PATCH] D54384: [llvm-objcopy] Add --build-id-link-dir flag

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

@mcgrathr 's comments are probably more relevant than mine - especially those that, by the sounds of it, remove the need for adding a new iterator (but I only found that after I'd already written some of that - and figured the comments might be useful general feedback/'learnings' anyway :) )



================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:131
+
+  SmallString<128> Path(Config.BuildIdLinkDir);
+  sys::path::append(Path, llvm::toHex(BuildIdBytes.getValue()[0], true));
----------------
Prefer = instead of () to make it clear you're not relying on an explicit ctor/conversion?

  SmallString<123> Path = Config.BuildIdLinkDir;


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:59
+public:
+  explicit NoteIterator() : Begin(nullptr), End(nullptr), Cur(nullptr) {}
+  explicit NoteIterator(const uint8_t *Begin, const uint8_t *End)
----------------
An Explicit default ctor seems a bit novel - is there anything this is particularly protecting against? (seems like it'd make it invalid to use {} to construct a NoteIterator - is that important?)

Also: Maybe use non-static data member initializers?

  const uint64_t *Begin = nullptr;
  const uint64_t *End = nullptr;
  const Note *Cur = nullptr;

(& then this ctor could be "NoteIterator() = default;")


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:62
+      : Begin(Begin), End(End) {
+    Cur = reinterpret_cast<const Note *>(Begin);
+  }
----------------
Might as well use the init list for Cur too?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:66-70
+    if (Begin >= End) {
+      Cur = nullptr;
+    } else {
+      Cur = reinterpret_cast<const Note *>(Begin);
+    }
----------------
Usually folks omit {} on single-line (or single statement) scopes in LLVM code.

(also could use a conditional operator, since each branch is an assignment to the same variable - but that's super subjective, of course :) )


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:73-74
+  }
+  bool operator==(const NoteIterator &Iter) const { return Cur == Iter.Cur; }
+  bool operator!=(const NoteIterator &Iter) const { return Cur != Iter.Cur; }
+  const Note &operator*() { return *Cur; }
----------------
Prefer non-member versions of operator overloads so any implicit conversions apply equally to the LHS and RHS? (they can still be friend definitions defined inline in the class definition here)

& maybe define one in terms of the other for consistency?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:83-86
+  const NoteIterator begin() const {
+    return NoteIterator{NoteData.begin(), NoteData.end()};
+  }
+  const NoteIterator end() const { return NoteIterator{}; }
----------------
Const qualifying value return types is a bit problematic, so far as I recall - doesn't allow the value to be moved from, etc, etc?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:90
+Optional<ArrayRef<uint8_t>> findBuildID(ArrayRef<uint8_t> NoteData) {
+  for (const auto &Note : NoteRange{NoteData}) {
+    if (Note.Type == NT_GNU_BUILD_ID && Note.name() == "GNU") {
----------------
I don't think there's much use of {} ctor invocation in LLVM, so I'd suggest using () for consistency.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:90
+Optional<ArrayRef<uint8_t>> findBuildID(ArrayRef<uint8_t> NoteData) {
+  for (const auto &Note : NoteRange{NoteData}) {
+    if (Note.Type == NT_GNU_BUILD_ID && Note.name() == "GNU") {
----------------
dblaikie wrote:
> I don't think there's much use of {} ctor invocation in LLVM, so I'd suggest using () for consistency.
Worth using std::find? Probably not, but figured I'd mention/ask in case.


Repository:
  rL LLVM

https://reviews.llvm.org/D54384