[PATCH] D148787: [llvm-debuginfo-analyzer] Add combined selection for logical elements.

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Sounds good, some questions / suggestions inline



================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:468
 If the <pattern> criteria is too general, a more selective option can
 be specified to target a particular category of elements:
 lines (:option:`--select-lines`), scopes (:option:`--select-scopes`),
----------------
Good place to forward-reference the filtering/combined section?


================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:579
 
+COMBINED
+^^^^^^^^
----------------
Could I suggest that this section is titled and described in terms of "filter" rather than "combined selection" -- I think the meaning of "filter" is clear to all readers, and it'll be easier + quicker to understand the command line options if written in those terms.

As far as I understand it, all of the options you're adding are to reduce the number of things printed by filtering for a particular condition, right?


================
Comment at: llvm/docs/CommandGuide/llvm-debuginfo-analyzer.rst:630
+
+For more specific combined criterias, a mix of the following options
+
----------------
What do you mean by mix here -- you can use multiple `--select-something` options, or you need to add pairs of `--select` and `--select-something`?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h:523-534
+    if (options().getSelectCombinedPattern()) {
+      if (CheckPattern() && (Requests.size() || ElementRequest.size()) &&
+          checkElementRequest(Element, Requests))
+        addElement(Element);
+    } else {
+      if ((options().getSelectGenericPattern() && CheckPattern()) ||
+          (options().getSelectOffsetPattern() && CheckOffset()) ||
----------------
It's not completely clear to me what's going on here (docu-comments appreciated), in particular, shouldn't the `getSelectCombinedPattern` case be a stricter version of the other side of the branch? And if so, why is it different, can't it be implemented as

    if (doesnt-match-filter)
        return;
    the-rest-of-the-body();

?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148787