[PATCH] D48501: [dsymutil] Introduce a new CachedBinaryHolder

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/tools/dsymutil/BinaryHolder.h:30
 
+class CachedBinaryHolder {
+public:
----------------
Could you add a Doxygen comment explaining what this class is for?


================
Comment at: llvm/tools/dsymutil/BinaryHolder.h:36
+
+  class EntryBase {
+  protected:
----------------
comment?


================
Comment at: llvm/tools/dsymutil/BinaryHolder.h:43
+
+  struct ObjectEntry : public EntryBase {
+    std::vector<std::unique_ptr<object::ObjectFile>> Objects;
----------------
comment?


================
Comment at: llvm/tools/dsymutil/BinaryHolder.h:44
+  struct ObjectEntry : public EntryBase {
+    std::vector<std::unique_ptr<object::ObjectFile>> Objects;
+
----------------
Could this be a `std::vector<object::ObjectFile>` instead?


================
Comment at: llvm/tools/dsymutil/BinaryHolder.h:58
+
+    /// Access to a derived version of all the currently owned ObjectFiles. The
+    /// conversion might be invalid, in which case an Error is returned.
----------------
As a casual reader it's no-obvious to me what a "derived version" could be.


================
Comment at: llvm/tools/dsymutil/BinaryHolder.h:87
+
+  struct ArchiveEntry : public EntryBase {
+    struct KeyTy {
----------------
comment? :-)


Repository:
  rL LLVM

https://reviews.llvm.org/D48501