[PATCH] D103220: Support stripping indirectly referenced DILocations from !llvm.loop metadata in stripDebugInfo()

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:390-391
+    return false;
+  if (Reachable.count(N) || isa<DILocation>(N))
+    return true;
+  if (!Visited.insert(N).second)
----------------
JDevlieghere wrote:
> Do we need to remember that the MDNode was visited when it's a DILocation? 
In that case it will be in `Reachable` so there's no need to.


================
Comment at: llvm/lib/IR/DebugInfo.cpp:411
   // if there is no debug location, we do not have to rewrite this MDNode.
-  if (std::none_of(N->op_begin() + 1, N->op_end(), [](const MDOperand &Op) {
-        return isa<DILocation>(Op.get());
+  if (!std::count_if(N->op_begin() + 1, N->op_end(), [&](const MDOperand &Op) {
+        return isa<DILocation>(Op.get()) ||
----------------
shafik wrote:
> I see in the other files the lambda explicitly list their captures instead of just using `&` to capture all by reference, I think that is a better style to be explicit.
For a one-line lambda like this, I feel like it just adds visual noise and makes it harder to read:
```
  if (!std::count_if(N->op_begin() + 1, N->op_end(),
   [&Visited, &DILocationReachable](const MDOperand &Op) {
     return isa<DILocation>(Op.get()) ||
               isDILocationReachable(Visited, DILocationReachable, Op.get());
      }))
```


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

https://reviews.llvm.org/D103220