[PATCH] D125780: [llvm-dva] 05 - Select elements

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added a comment.

Good job! Here is a series of comments, please have a look if you don't mind.



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVElement.h:62
 using LVElementKindSet = std::set<LVElementKind>;
+using LVElementDispatch = std::map<LVElementKind, LVElementGetFunction>;
+using LVElementRequest = std::vector<LVElementGetFunction>;
----------------
Could you add `#include <map>` and `#include <vector>` to make the header more self-contained?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:19
 #include "llvm/DebugInfo/LogicalView/Core/LVLine.h"
 #include "llvm/DebugInfo/LogicalView/Core/LVOptions.h"
 #include "llvm/DebugInfo/LogicalView/Core/LVScope.h"
----------------
Do you mean `LVObject.h`? Currently it looks like a self-inclusion.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:452
 
+class LVPatterns {
+  // Pattern Mode.
----------------
The class should either be marked as `final` or has a virtual destructor.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:470
+  LVMatchInfo GenericMatchInfo;
+  using LVMatchOffset = std::vector<uint64_t>;
+  LVMatchOffset OffsetMatchInfo;
----------------
Maybe it makes sense to rename the alias into `LVMatchOffsets` with `s` because the code already has `LVOffset` alias for just `uint64_t` and it could get the reader in doubt why an instance of `LVMatchOffset` is a heavy object.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:490
+  bool checkElementRequest(const T *Element, const U &Requests) const {
+    for (const auto &Request : Requests)
+      if ((Element->*Request)())
----------------
What about to add an `assert` to check that `Element` is not null?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:502
+  template <typename T, typename U, typename V>
+  void addRequest(const T &Selection, U &Dispatch, V &Request) const {
+    for (auto &Entry : Selection) {
----------------
I see no modifications of `Dispatch` in the function.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:503
+  void addRequest(const T &Selection, U &Dispatch, V &Request) const {
+    for (auto &Entry : Selection) {
+      // Find target function to fullfit request.
----------------



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:515
+  void resolveGenericPatternMatch(T *Element, const U &Requests) {
+    auto checkPattern = [&]() -> bool {
+      return Element->isNamed() &&
----------------
What about to add an assert to check that `Element` is not null? Also it looks like the `Element` could have the `const T *` type.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:531
+  template <typename U>
+  void resolveGenericPatternMatch(LVLine *Line, const U &Requests) {
+    auto checkPattern = [&]() -> bool {
----------------
What about to add an assert to check that Element is not null? 


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:532
+  void resolveGenericPatternMatch(LVLine *Line, const U &Requests) {
+    auto checkPattern = [&]() -> bool {
+      return matchGenericPattern(Line->lineNumberAsStringStripped()) ||
----------------
The lambda captures `this` and a pointer (`Line`) only, it makes sense to capture `this` by reference (implicitly) and `Line` by value: `auto checkPattern = [=]() -> bool {` (will this ought to be replaced with `[=, this]` when LLVM switches on C++ 20?)


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:537
+    };
+    auto checkOffset = [&]() -> bool {
+      return matchOffsetPattern(Line->getAddress());
----------------
The previous comment for `checkPattern` could be applied here too.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:561
+  LVPatterns(const LVPatterns &) = delete;
+  LVPatterns &operator=(LVPatterns const &) = default;
+  ~LVPatterns() = default;
----------------
A typo? `= delete` should be?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:597
+
+  bool matchPattern(StringRef Input, LVMatchInfo &MatchInfo);
+  // Match a pattern (--select='pattern').
----------------
Would `const LVMatchInfo &MatchInfo` work?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:602
+  }
+  bool matchOffsetPattern(uint64_t Offset) {
+    return std::find(OffsetMatchInfo.begin(), OffsetMatchInfo.end(), Offset) !=
----------------
The `LVObject.h` header defines an alias `LVOffset` for `uint64_t`.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:646
+
+inline LVPatterns &patterns() { return (*LVPatterns::getPatterns()); }
+
----------------
I believe this pair of brackets is not needed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVOptions.cpp:447
+
+void LVPatterns::addOffsetPatterns(LVOffsetSet &Patterns) {
+  for (const uint64_t &Entry : Patterns)
----------------
`Patterns` has not been changed in the method.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVOptions.cpp:448
+void LVPatterns::addOffsetPatterns(LVOffsetSet &Patterns) {
+  for (const uint64_t &Entry : Patterns)
+    OffsetMatchInfo.push_back(Entry);
----------------



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVOptions.cpp:509
+  // Traverse all match specifications.
+  for (LVMatch &Match : MatchInfo) {
+    switch (Match.Mode) {
----------------



================
Comment at: llvm/unittests/DebugInfo/LogicalView/SelectElementsTest.cpp:53
+    // have the ability of kind selection.
+    T *Element = new T();
+    EXPECT_NE(Element, nullptr);
----------------
An `std::bad_alloc` will be thrown if the test is out of memory. It makes sense to wrap the line in the `try-catch` block or use the non-throwing overload of the `new` operator.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/SelectElementsTest.cpp:92
+  Element->setType(Type);
+  EXPECT_STREQ(Element->getName().data(), Name.data());
+  EXPECT_EQ(Element->getOffset(), Offset);
----------------
It would be better to use `EXPECT_EQ` on the objects of `Element->getName()` and `Name` because the `.data()` member of `StringRef` doesn't guarantee that the returned char array will be null-terminated.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/SelectElementsTest.cpp:276-277
+  EXPECT_EQ(Aggregate->getIsMatched(), 0);
+  EXPECT_EQ(Function->getIsMatched(), 0);
+  EXPECT_EQ(NestedScope->getIsMatched(), 1);
+
----------------
If I get it right, `getIsMatched()` returns a `bool` value. GTest's `EXPECT_TRUE` and `EXPECT_FALSE` can handle `bool` values better.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/SelectElementsTest.cpp:368
+  EXPECT_EQ(LocalVariable->getIsMatched(), 1);  // LocalVariable
+  EXPECT_EQ(NestedVariable->getIsMatched(), 1); // NesteVariable
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125780