[Lldb-commits] [PATCH] D77186: [source maps] Ensure all valid source maps are added instead of failing with the first invalid one

Thu Apr 2 01:36:01 PDT 2020

labath added a comment.

In D77186#1955640 <https://reviews.llvm.org/D77186#1955640>, @wallace wrote:

> Thanks for the heads up. I think I got a false impression of how comments after accept vs requesting changes work in this repo.

The rules on that are somewhat fuzzy, but there has been a recent effort to formalize <http://llvm.org/docs/CodeReview.html> that. Generally, in a situation like this (where a patch already got one LGTM and so is "green"), I would have used "request changes" if I had major objections and was sure I want to see the patch again. If I only had cosmetic comments/trivial comments, I would have clicked "approve" and trusted you to apply them before committing. Not doing either of those is a sort of a "meh" position, where I'm uncomfortable lgtm-ing something, but I also don't want to reject it (as that's fairly aggressive). In this case, I don't think you have done anything really wrong strictly speaking, but it would have been polite to wait a while so that I have a chance to respond.

================
Comment at: lldb/source/Interpreter/OptionValuePathMappings.cpp:73
             changed = true;
+            idx++;
           } else {
----------------
wallace wrote:
> labath wrote:
> > does this actually change anything?
> Indeed. It's used in line 70 as the index to replace in the replace operation
Yes, but that would have also worked (I think) if you had left the increment in the for loop. My question was why did you need to move the increment into the loop body.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77186