[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Zachary Turner via lldb-commits]

On Wed, Aug 15, 2018 at 4:48 PM Greg Clayton via Phabricator <
reviews at reviews.llvm.org> wrote:

> clayborg marked 23 inline comments as done.
> clayborg added inline comments.
>
>
> ================
> Comment at: tools/lldb-vscode/SourceBreakpoint.h:24
> +  // Set this breakpoint in LLDB as a new breakpoint
> +  void SetBreakpoint(const char *source_path);
> +};
> ----------------
> zturner wrote:
> > clayborg wrote:
> > > zturner wrote:
> > > > `StringRef source_path`
> > > I am gonna leave this one as is because we must pass a "const char *"
> the API that is called inside the body of this method:
> > >
> > > ```
> > >   lldb::SBBreakpoint lldb::SBTarget::BreakpointCreateByLocation(const
> char *file, uint32_t line);
> > > ```
> > >
> > In that case you can use `const llvm::Twine &`.  You can pass many types
> of objects to it, such as `const char*`, `std::string`, `llvm::StringRef`.
> Then, inside the function you can write:
> >
> > ```
> > llvm::SmallString<32> S;
> > StringRef NTS = source_path.toNullTerminatedStringRef(S);
> > ```
> >
> > If the original parameter was constructed with a `const char*` or
> `std::string`, then `toNullTerminatedStringRef` is smart enough to know
> that it doesn't need to make a copy, and it just returns the pointer.  If
> it was constructed with a `StringRef`, then it null terminates it using `S`
> as the backing storage and returns that pointer.  So it's the best of both
> worlds
> That is just way too much work for no gain.


The gain is that const char* is unsafe and error prone and should be
avoided wherever possible unless it’s very cumbersome.

>
> ================
> Comment at: tools/lldb-vscode/VSCode.h:61
> +  FILE *in;
> +  FILE *out;
> +  lldb::SBDebugger debugger;
> ----------------
> This can work with a port number. Rather not switch away from FILE * for
> now. No real benefit.


What can work with a port number?  Currently these are initialized to stdin
and stdout and never change.  There is also significant benefit in terms of
simplicity, readability, and code maintain ability.  The entire fgets loop
goes away and is replaced with a LineIterator which occupies a fraction of
tge amount of code while being safer.  You can use more expressive (and
safer) formatting constructs.

I would turn this around and say that there’s no real benefit to NOT
changing this, so we should prefer more modern and safer constructs unless
there is a good reason not to

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180815/3a7dbbdb/attachment.html>