[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:59 PM Zachary Turner <zturner at google.com> wrote:

>
>
> 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.
>
I forgot inflexible.  As soon as a function takes a const char *, it starts
trickling through the interface.  Now callers of the function or other
member variables become const char *.  Then it goes out further.
Eventually you end up with it being spread out throughout your whole
program because of one function.  I might be exaggerating slightly, but
only slightly. The point is, Interface design should be orthogonal to
implementation.

It might be a minor nitpick, but we have a basically clean slate here so I
would prefer to start on the right foot with modern c++ constructs and
paradigms.

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