[Lldb-commits] [PATCH] D76968: [lldb-vscode] Correctly return source mapped breakpoints for setBreakpoints request

Tue Mar 31 01:04:09 PDT 2020

labath added a comment.

In D76968#1951707 <https://reviews.llvm.org/D76968#1951707>, @clayborg wrote:

> Looks good as long as with we have "llvm::Optional<llvm::StringRef> request_path = {}" in the arguments for a function, that "{}" will turn into llvm::None and not an empty StringRef? Unclear to me. As long as this is what is happening and as long is this coding convention is used elsewhere in llvm (using "{}" instead of "llvm::None") I am ok. Pavel?

The two expressions are equivalent here. We're currently using both styles in lldb. I have a slight preference for the explicit None version, but I don't think it's worth the trouble to standardize on one or the other.

================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1974
       // Add this breakpoint info to the response
       AppendBreakpoint(pair.second.bp, response_breakpoints);
     }
----------------
wallace wrote:
> labath wrote:
> > What about breakpoints in dynamically loaded shared libraries? Should you be remembering the original path somewhere so that one you can set it here too?
> This case is different, as function breakpoints are set by the IDE without referring to any source path, as they are equivalent to making 'b function_name'. Probably function breakpoints are not working correctly, so I'll have to fix it any of these days using the expensive query I mentioned above.
Ah, sorry. I misplaced this comment. It was meant to go above to line 417, where the `eBreakpointEventTypeLocationsAdded` event is handled. That code should be reached for "regular" file+line breakpoints in case a breakpoint gets new locations (e.g.) in response to a shared library being loaded.

================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:421
             llvm::json::Object body;
             body.try_emplace("breakpoint", CreateBreakpoint(bp));
             body.try_emplace("reason", "changed");
----------------
The comment was supposed to be here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76968