[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/LVSymbol.h:180
+  bool equals(const LVSymbol *Symbol) const;
+  static bool equals(const LVSymbols *References, const LVSymbols *Targets);
+  void report(LVComparePass Pass) override;
----------------
probinson wrote:
> Comment does not apply to the second overload.
> Also curious that this time the second overload is `static` when for other classes it is not.
Added comments to second method.

All `bool equals(const LVAbc *References, const LVAbc *Targets)` methods in all the classes are static.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVType.h:134
+  virtual bool equals(const LVType *Type) const;
+  static bool equals(const LVTypes *References, const LVTypes *Targets);
+  void report(LVComparePass Pass) override;
----------------
probinson wrote:
> Comment does not apply to the second overload.
> Which is also `static` ?
Added comment to second method.

All `bool equals(const LVAbc *References, const LVAbc *Targets)` methods in all the classes are static.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:861
+
+// Returns true if 'current' scope is equal to the given 'scope'.
+bool LVScope::equalNumberOfChildren(const LVScope *Scope) const {
----------------
probinson wrote:
> 
Moved the comment to the include file and changed the comment.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1054
+
+LVScope *LVScopeAggregate::equals(const LVScopes *Scopes) const {
+  assert(Scopes && "Scopes must not be nullptr");
----------------
probinson wrote:
> The name `equals` implies a straightforward equality test, which is not really what this method does.  In fact it's a little hard for me to describe what it does...  looks through a given set of Scopes to find one that equals the current Scope?  So maybe `findEqualScope` would be a better name?
- Renamed method to `findEqualScope`.
- Added comment to method declarations.

```
  // For the given 'Scopes' returns a scope that is logically equal
  // to the current scope; otherwise 'nullptr'.
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1807
+
+LVScope *LVScopeFunction::equals(const LVScopes *Scopes) const {
+  assert(Scopes && "Scopes must not be nullptr");
----------------
probinson wrote:
> Same comment as `LVScopeAggregate::equals`
- Renamed method to `findEqualScope`.
- Added comment to method declarations.

```
  // For the given 'Scopes' returns a scope that is logically equal
  // to the current scope; otherwise 'nullptr'.
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1876
+
+LVScope *LVScopeFunctionInlined::equals(const LVScopes *Scopes) const {
+  return LVScopeFunction::equals(Scopes);
----------------
probinson wrote:
> Same comment as `LVScopeAggregate::equals`
- Renamed method to `findEqualScope`.
- Added comment to method declarations.

```
  // For the given 'Scopes' returns a scope that is logically equal
  // to the current scope; otherwise 'nullptr'.
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1939
+
+LVScope *LVScopeNamespace::equals(const LVScopes *Scopes) const {
+  assert(Scopes && "Scopes must not be nullptr");
----------------
probinson wrote:
> Same comment as `LVScopeAggregate::equals`
- Renamed method to `findEqualScope`.
- Added comment to method declarations.

```
  // For the given 'Scopes' returns a scope that is logically equal
  // to the current scope; otherwise 'nullptr'.
```


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

https://reviews.llvm.org/D125782