[PATCH] D125782: [llvm-debuginfo-analyzer] 07 - Compare elements

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h:35
+  // the comparison pass.
+  // By recording the missing/added elements along with its pass, it
+  // allow to check which elements were missing/added during each pass.
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h:36
+  // By recording the missing/added elements along with its pass, it
+  // allow to check which elements were missing/added during each pass.
+  LVPassTable PassTable;
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h:62
+
+public:
+  static LVCompare &getInstance();
----------------
probinson wrote:
> No need to repeat `public:`
Removed redundant `public:`.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVElement.h:345
+  // Report the element as missing or added during comparison.
+  virtual void report(LVComparePass Pass){};
+
----------------
probinson wrote:
> 
Changed to `virtual void report(LVComparePass Pass) {}`


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:300
+
+  // Returns true if 'current' scope is equal to the given 'scope'.
+  virtual bool equalNumberOfChildren(const LVScope *Scope) const;
----------------
probinson wrote:
> Does this comment apply to all the methods in this block of code?  It seems like not, in which case it would be good to put a blank line before the declarations that it does not apply to.  (Or even better, comments for each of the other methods!)
Added comments to each method.



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:307
+
 public:
   static LVScopeDispatch &getDispatch() { return Dispatch; }
----------------
probinson wrote:
> No need to repeat `public:`
Removed redundant `public:`.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:351
+  bool equals(const LVScope *Scope) const override;
+  LVScope *equals(const LVScopes *Scopes) const override;
+
----------------
probinson wrote:
> The comment does not describe the second overload; put in a blank line, or preferably a comment for the second one.
Added comment to second method.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:652
+  bool equals(const LVScope *Scope) const override;
+  LVScope *equals(const LVScopes *Scopes) const override;
+
----------------
probinson wrote:
> Comment does not apply to the second overload.
Added comment to second method.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:692
+  bool equals(const LVScope *Scope) const override;
+  LVScope *equals(const LVScopes *Scopes) const override;
+
----------------
probinson wrote:
> Comment does not apply to the second overload.
Added comment to second method.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:730
+  bool equals(const LVScope *Scope) const override;
+  LVScope *equals(const LVScopes *Scopes) const override;
+
----------------
probinson wrote:
> Comment does not apply to the second overload.
Added comment to second method.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h:183
+
 public:
   void print(raw_ostream &OS, bool Full = true) const override;
----------------
probinson wrote:
> no need to keep repeating `public:`
Remove redundand `public:`.


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

https://reviews.llvm.org/D125782