[PATCH] D38719: [llvm-dwarfdump] Verify compatible TAG for attributes.

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere marked an inline comment as done.
JDevlieghere added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:115-119
+DWARFUnit *DWARFUnitVector::addUnit(std::unique_ptr<DWARFUnit> Unit) {
+  this->push_back(std::move(Unit));
+  return this->back().get();
+}
+
----------------
dblaikie wrote:
> This probably needs to ensure the newly added unit is added such that the vector remains in sorted order (so getUnitForOffset, etc, can rely on that sorted order for offset searches).
> 
> It could assert this (if the caller(s) you've added ensure this is true) or do a search/insert to keep things sorted.
> 
> Or, if you know you won't be using getUnitForOffset, etc, I guess it doesn't need to. Or if you have a phase where you're building the vector then a separate phase where you're using getUnitForOffset - you could add them all, then sort, then query - that's a bit more efficient that maintaining sorted order when you don't need it.
I've updated the patch to keep thing sorted, it seems like the least bug-prone as we keep the invariant. In the verifier we also build and use the vector at the same time, so sorting at the end is tricky (and would require more state if we want to enforce this, making it a less localized approach than simply sorting). 


https://reviews.llvm.org/D38719