[PATCH] D125778: [llvm-dva] 03 - Logical elements

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:68
+protected:
+  LVScopeRoot *Root = nullptr;
+  std::string InputFilename;
----------------
psamolysov wrote:
> Why `Root` cannot be a smart pointer in order to not use the `new/delete` machinery below?
Before uploading the patches we discussed the `smart pointer` usage across the tool but we decided to wait until all patches are accepted. After that. the next step is to remove the `new/delete` machinary across the tool. 


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:221
+
+  virtual void resolve() override;
+  virtual void resolveName() override;
----------------
probinson wrote:
> psamolysov wrote:
> > Either `virtual` or `override` is enough.
> When a declaration overrides an inherited `virtual` declaration, it really should use `override` to guarantee that the prior `virtual` declaration exactly matches.  `override` implies `virtual` and so we omit `virtual` to reduce clutter.
Omitting `virtual`.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:260
+  }
+  virtual void setReference(LVElement *Element) override {
+    setReference(static_cast<LVScope *>(Element));
----------------
psamolysov wrote:
> `setReference(LVScope *)` above is not marked as `virtual`, only `override` is used.
Omitting `virtual`.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:476-477
+
+  virtual void resolveExtra() override;
+  virtual void resolveReferences() override;
+
----------------
psamolysov wrote:
> 
Omitting `virtual`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125778