[PATCH] D125784: [llvm-debuginfo-analyzer] 09 - CodeView Reader

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:422
+  Expected<InfoStream &> expectedInfo = Pdb.getPDBInfoStream();
+  if (expectedInfo && expectedInfo->getGuid() != TS.getGuid())
+    return createStringError(errc::invalid_argument, "signature_out_of_date");
----------------
probinson wrote:
> If we have to check `expectedInfo` anyway, let's not assume `getPDBInfoStream` succeeds.
Good suggestion. Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:226
+  using LookupSet = std::set<StringRef>;
+  LookupSet DeductedScopes;
+  LookupSet UnresolvedScopes;
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:3404
+      break;
+    }
+  }
----------------
probinson wrote:
> Isn't this `for` loop (which does a `break` on the first iteration) equivalent to
> ```
> if (!Locations->empty())
>   ParentLowPC = Locations->begin()->getLowerAddress();
> ```
Good finding. Replaced with
```
  if (const LVLocations *Locations = Parent->getRanges()) {
    if (!Locations->empty())
      ParentLowPC = (*Locations->begin())->getLowerAddress();
  }
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:3496
+        SeenHighAddress = true;
+        break;
+      }
----------------
probinson wrote:
> `break` not needed here
Removed.


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

https://reviews.llvm.org/D125784