[PATCH] D125777: [llvm-dva] 02 - Driver and documentation

Thu Jun 23 03:43:14 PDT 2022

CarlosAlbertoEnciso added inline comments.

================
Comment at: llvm/docs/CommandGuide/llvm-dva.rst:239
+ extended knowledge about debug information. They are intended to be
+ used when a more low level detail is required.
+
----------------
probinson wrote:
> psamolysov wrote:
> > CarlosAlbertoEnciso wrote:
> > > CarlosAlbertoEnciso wrote:
> > > > CarlosAlbertoEnciso wrote:
> > > > > psamolysov wrote:
> > > > > > details?
> > > > > May be `used when a more low level details are required.`
> > > > Even better: `used when lower level of detail is required.`
> > > Replaced with: `used when lower level detail is required.`
> > The proposed wording looks good for me. Thank you.
> when a lower level of detail is required.
Final sentence:
`They are intended when a lower level of detail is required.`

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVElement.h:27
+
+class LVElement : public LVObject {
+  // Indexes in the String Pool.
----------------
psamolysov wrote:
> Does it make sense to mark the `LVElement` class as `final`?
Marking `LVElement` as `final` it would be OK for the current patch. But then we need to remove it in the next patch. Another point is that the current patch does not create any instances of `LVElement`.

It is just the driver and documentation.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:17
+
+#include "llvm/DebugInfo/LogicalView/Core/LVSupport.h"
+
----------------
psamolysov wrote:
> Let's include `<string>` to make the file self contained.
Including `<string>`.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:53
+  std::string lineNumberAsStringStripped(bool ShowZero = false) const;
+};
+
----------------
psamolysov wrote:
> The virtual destructor is required, isn't it?
I see your point. What this patch adds is just the driver and the documentation. No logical elements are created; basically no instances of `LVObject`, `LVElement`, `LVLine`, ``LVScope` or `LVSymbol` are created.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:23
+#include "llvm/DebugInfo/LogicalView/Core/LVType.h"
+
+namespace llvm {
----------------
psamolysov wrote:
> Let's include `<set>` and `<string>` to make the file self contained.
`<string>` already included by `LVObject.h`. See previous change.
`<set>` already included by `LVElement`.

May be I am not understanding your point. Do you prefer those includes to be here instead of being include indirectly?

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:300-303
+  LVOptions() = default;
+  LVOptions(const LVOptions &) = default;
+  LVOptions &operator=(LVOptions const &) = default;
+  ~LVOptions() = default;
----------------
psamolysov wrote:
> Just for interest, why do you explicitly define these default members?
Just to explicitly state that these members use the default implementation.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:19
+#include "llvm/DebugInfo/LogicalView/Core/LVSort.h"
+
+namespace llvm {
----------------
psamolysov wrote:
> Let's include <set> to make the file self contained.
`<set>` already included by `LVElement`.

May be I am not understanding your point. Do you prefer this include to be here instead of being include indirectly?

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h:29
+class LVStringPool {
+  const size_t BadIndex = std::numeric_limits<size_t>::max();
+  using TableType = StringMap<size_t, BumpPtrAllocator>;
----------------
psamolysov wrote:
> Can we use `constexpr` here (I'm not sure is it available in C++14)?
But then `BadIndex` should be declared as `static`. Changed to `static constexpr size_t BadIndex`.

================
Comment at: llvm/tools/llvm-dva/Options.cpp:394-395
+        clEnumValN(LVTypeKind::IsEnumerator, "Enumerator", "Enumerator."),
+        clEnumValN(LVTypeKind::IsImport, "Import", "Import declaration."),
+        clEnumValN(LVTypeKind::IsImportDeclaration, "ImportDeclaration",
+                   "Import declaration."),
----------------
psamolysov wrote:
> Both options are described as `Import declaration`. Is this actual?
`clEnumValN(LVTypeKind::IsImport, "Import", "Import declaration."),`
should be
`clEnumValN(LVTypeKind::IsImport, "Import", "Import."),`

================
Comment at: llvm/tools/llvm-dva/Options.cpp:465
+  // Traverse list of options and update the given set (Using case and Regex).
+  auto updatePattern = [&](auto &List, auto &Set, bool &Case, bool &Regex) {
+    if (!List.empty())
----------------
psamolysov wrote:
> `Case` and `Regex` parameters are `bool`s and can be passed by value, why are they passed by reference?
Not a particular reason. Passing them by value.

================
Comment at: llvm/tools/llvm-dva/Options.cpp:468
+      for (std::string &Pattern : List)
+        Set.insert((Case && !Regex) ? StringRef(Pattern).lower() : Pattern);
+  };
----------------
psamolysov wrote:
> `StringRef(Pattern).lower()` changes the string in the `List` or I'm missing something. Is it expected?
The behaviour is correct. The string is converted to lowercase. May be the issue is the parameter name `Case` that controls the conversion.
Changed the parameters to `IgnoreCase` and `UseRegex`.

================
Comment at: llvm/tools/llvm-dva/Options.cpp:476
+  // Traverse list of options and update the given set.
+  auto updateSet = [&](auto &List, auto &Set) {
+    if (!List.empty())
----------------
psamolysov wrote:
> I believe it can be replaced with `std::copy` or the corresponding overload of the `insert` method of the `Set` could be used.
Replaced with `std::copy(List.begin(), List.end(), std::inserter(Set, Set.begin()));`.

================
Comment at: llvm/tools/llvm-dva/Options.h:27
+
+class OffsetParser : public llvm::cl::parser<unsigned long long> {
+public:
----------------
psamolysov wrote:
> Should the class be marked as `final`?
Marked as `final`.

================
Comment at: llvm/tools/llvm-dva/llvm-dva.cpp:71
+  }
+  if (!BundlePaths.size())
+    BundlePaths.push_back(InputPath);
----------------
psamolysov wrote:
> 
Replaced with `empty()`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125777