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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
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()) ||
----------------
jmorse wrote:
> 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();
> 
> ?
'SelectCombinedPattern` is set when:

(`--select-elements` **or** `--select-lines` **or** `--select-scopes` **or** `--select-symbols` **or** `--select-types`) **and** (`--select`) are specified in the command line.

The matching conditions for both branches are different.
I can remove the `else` branch and add `return` on the `if` branch to simplify the code. Basically the `combined` takes priority over the normal selection.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148787