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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
