<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">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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
================<br>
Comment at: tools/lldb-vscode/VSCode.h:61<br>
+  FILE *in;<br>
+  FILE *out;<br>
+  lldb::SBDebugger debugger;<br>
----------------<br>
This can work with a port number. Rather not switch away from FILE * for now. No real benefit.</blockquote><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">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</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote></div></div>