[PATCH] D125780: [llvm-dva] 05 - Select elements
Carlos Alberto Enciso via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 1 04:08:23 PDT 2022
CarlosAlbertoEnciso added inline comments.
================
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>;
----------------
psamolysov wrote:
> Could you add `#include <map>` and `#include <vector>` to make the header more self-contained?
Included `<map>` and `<vector>`.
================
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"
----------------
psamolysov wrote:
> Do you mean `LVObject.h`? Currently it looks like a self-inclusion.
Very good catch.
`LVObject.h` it is not needed as it is included by any of the logical elements.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:452
+class LVPatterns {
+ // Pattern Mode.
----------------
psamolysov wrote:
> The class should either be marked as `final` or has a virtual destructor.
Added `final`.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:470
+ LVMatchInfo GenericMatchInfo;
+ using LVMatchOffset = std::vector<uint64_t>;
+ LVMatchOffset OffsetMatchInfo;
----------------
psamolysov wrote:
> 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.
Renamed to `LVMatchOffsets`.
================
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)())
----------------
psamolysov wrote:
> What about to add an `assert` to check that `Element` is not null?
Added `assert(Element && "Element must not be nullptr");`
================
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) {
----------------
psamolysov wrote:
> I see no modifications of `Dispatch` in the function.
Added `const`.
================
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.
----------------
psamolysov wrote:
>
Added `const`.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:515
+ void resolveGenericPatternMatch(T *Element, const U &Requests) {
+ auto checkPattern = [&]() -> bool {
+ return Element->isNamed() &&
----------------
psamolysov wrote:
> 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.
Added `assert(Element && "Element must not be nullptr");`
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:515
+ void resolveGenericPatternMatch(T *Element, const U &Requests) {
+ auto checkPattern = [&]() -> bool {
+ return Element->isNamed() &&
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > 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.
> Added `assert(Element && "Element must not be nullptr");`
>
I can add the `const` qualifier, but as side effect a lot of functions needs to be modified as they are not expected a `const`. They are indirectly called by `addElement(Element)`.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:531
+ template <typename U>
+ void resolveGenericPatternMatch(LVLine *Line, const U &Requests) {
+ auto checkPattern = [&]() -> bool {
----------------
psamolysov wrote:
> What about to add an assert to check that Element is not null?
Added `assert(Line && "Line must not be nullptr");`
================
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()) ||
----------------
psamolysov wrote:
> 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?)
Thanks for the explanation.
Changed to `auto CheckPattern = [=]() -> bool {`
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:537
+ };
+ auto checkOffset = [&]() -> bool {
+ return matchOffsetPattern(Line->getAddress());
----------------
psamolysov wrote:
> The previous comment for `checkPattern` could be applied here too.
Changed to `auto CheckOffset = [=]() -> bool {`
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:561
+ LVPatterns(const LVPatterns &) = delete;
+ LVPatterns &operator=(LVPatterns const &) = default;
+ ~LVPatterns() = default;
----------------
psamolysov wrote:
> A typo? `= delete` should be?
Good catch. Changed to `= delete`.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:597
+
+ bool matchPattern(StringRef Input, LVMatchInfo &MatchInfo);
+ // Match a pattern (--select='pattern').
----------------
psamolysov wrote:
> Would `const LVMatchInfo &MatchInfo` work?
Added `const`.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:602
+ }
+ bool matchOffsetPattern(uint64_t Offset) {
+ return std::find(OffsetMatchInfo.begin(), OffsetMatchInfo.end(), Offset) !=
----------------
psamolysov wrote:
> The `LVObject.h` header defines an alias `LVOffset` for `uint64_t`.
Changed.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:646
+
+inline LVPatterns &patterns() { return (*LVPatterns::getPatterns()); }
+
----------------
psamolysov wrote:
> I believe this pair of brackets is not needed.
Removed the wrapping brackets.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVOptions.cpp:447
+
+void LVPatterns::addOffsetPatterns(LVOffsetSet &Patterns) {
+ for (const uint64_t &Entry : Patterns)
----------------
psamolysov wrote:
> `Patterns` has not been changed in the method.
Added `const`.
================
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);
----------------
psamolysov wrote:
>
Replaced.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVOptions.cpp:509
+ // Traverse all match specifications.
+ for (LVMatch &Match : MatchInfo) {
+ switch (Match.Mode) {
----------------
psamolysov wrote:
>
Added `const`.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/SelectElementsTest.cpp:53
+ // have the ability of kind selection.
+ T *Element = new T();
+ EXPECT_NE(Element, nullptr);
----------------
psamolysov wrote:
> 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.
Changed to use the non-throwing overload.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/SelectElementsTest.cpp:92
+ Element->setType(Type);
+ EXPECT_STREQ(Element->getName().data(), Name.data());
+ EXPECT_EQ(Element->getOffset(), Offset);
----------------
psamolysov wrote:
> 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.
Changed to EXPECT_EQ(Element->getName(), Name);
================
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);
+
----------------
psamolysov wrote:
> If I get it right, `getIsMatched()` returns a `bool` value. GTest's `EXPECT_TRUE` and `EXPECT_FALSE` can handle `bool` values better.
Replaced with `EXPECT_TRUE` and `EXPECT_FALSE`.
================
Comment at: llvm/unittests/DebugInfo/LogicalView/SelectElementsTest.cpp:368
+ EXPECT_EQ(LocalVariable->getIsMatched(), 1); // LocalVariable
+ EXPECT_EQ(NestedVariable->getIsMatched(), 1); // NesteVariable
+
----------------
psamolysov wrote:
>
Fix typo.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125780/new/
https://reviews.llvm.org/D125780
More information about the llvm-commits
mailing list