[Lldb-commits] [PATCH] D49334: [LLDB} Added syntax highlighting support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for the explanation. This looks fine to me, though I would feel better if someone else gave it a look too.



================
Comment at: source/Core/Highlighter.cpp:29
+  if (!m_prefix.empty())
+    s << lldb_utility::ansi::FormatAnsiTerminalCodes(m_prefix);
+  s << value;
----------------
labath wrote:
> This call isn't exactly cheap. Maybe you could just call the function once during initialization and just store the result?
extra semicolons


================
Comment at: source/Core/Highlighter.cpp:34
+  // Calculate how many bytes we have written.
+  return m_prefix.size() + value.size() + m_suffix.size();
+}
----------------
teemperor wrote:
> labath wrote:
> > This isn't correct, as you're not writing m_prefix, but it's transmogrified version. Btw, do you really need this return value anyway?
> Good catch. And the return value is just to make the SourceManager happy which always returns the total amount of bytes written. I'm working on a patch that will move all the 'written byte counting' in lldb into the Stream class, but as of now that's how it works.
That sounds like a good idea. When you do that, could you please refer to the llvm raw_ostream classes to see how they handle this. Long term, it would be great if we could replace lldb's classes with those, so I'd like to avoid diverging from them if possible.


https://reviews.llvm.org/D49334