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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The patch is bigger than ideal for a single change, but I like the way it is structured. Thank you for abstracting the clang specifics.

The one high-level question that came to mind when looking this over is whether we really need to do all this file name matching to get the language type. I would expect we normally display the source code we have debug info for. Is that ever not the case? If it is, then we should be able to just get the language from the debug info and avoid relying on the file names. Have you looked at whether that is possible?



================
Comment at: include/lldb/Core/Highlighter.h:49
+    /// \return
+    ///     The number of bytes that have been written to the given string.
+    std::size_t Apply(Stream &s, llvm::StringRef value) const;
----------------
given **stream**


================
Comment at: include/lldb/Core/Highlighter.h:149
+  /// \param language_type
+  ///     The language type hat the caller thinks the source code was given in.
+  /// \param path
----------------
s/hat/that/


================
Comment at: source/Core/Highlighter.cpp:16-17
+
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/StringSet.h"
----------------
Are these clang includes used here? If they are, this entire abstraction exercise will have been for nothing.


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


================
Comment at: source/Core/Highlighter.cpp:34
+  // Calculate how many bytes we have written.
+  return m_prefix.size() + value.size() + m_suffix.size();
+}
----------------
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?


================
Comment at: source/Plugins/Language/ClangCommon/ClangHighlighter.cpp:21
+
+#include <unordered_set>
+
----------------
Is this used?


================
Comment at: source/Plugins/Language/ClangCommon/ClangHighlighter.cpp:155
+  std::unique_ptr<llvm::MemoryBuffer> buf =
+      llvm::MemoryBuffer::getMemBufferCopy(full_source);
+
----------------
Do you need to make a copy of the data here? It looks like both of these objects get destroyed at the end of this function anyway.


================
Comment at: unittests/Language/Highlighting/HighlighterTest.cpp:132-133
+
+  std::string output = highlightC("", s);
+  EXPECT_STREQ("", output.c_str());
+}
----------------
You can just do `EXPECT_EQ("", highlightC("", s))`. EXPECT_EQ uses operator==, which does the right thing when one of the args is a std::string.


https://reviews.llvm.org/D49334