<div><br></div><div><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 15, 2018 at 4:59 PM Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div dir="auto"><br></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 15, 2018 at 4:48 PM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg marked 23 inline comments as done.<br>
clayborg added inline comments.<br>
<br>
<br>
================<br>
Comment at: tools/lldb-vscode/SourceBreakpoint.h:24<br>
+  // Set this breakpoint in LLDB as a new breakpoint<br>
+  void SetBreakpoint(const char *source_path);<br>
+};<br>
----------------<br>
zturner wrote:<br>
> clayborg wrote:<br>
> > zturner wrote:<br>
> > > `StringRef source_path`<br>
> > 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:<br>
> > <br>
> > ```<br>
> >   lldb::SBBreakpoint lldb::SBTarget::BreakpointCreateByLocation(const char *file, uint32_t line);<br>
> > ```<br>
> > <br>
> 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:<br>
> <br>
> ```<br>
> llvm::SmallString<32> S;<br>
> StringRef NTS = source_path.toNullTerminatedStringRef(S);<br>
> ```<br>
> <br>
> 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<br>
That is just way too much work for no gain.</blockquote><div dir="auto"><br></div><div dir="auto">The gain is that const char* is unsafe and error prone and should be avoided wherever possible unless it’s very cumbersome.</div></div></div></blockquote><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">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.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote></div></div>
</blockquote></div></div>