[PATCH] Add new tool clang-highlight to clang/tools/extra
Daniel Jasper
djasper at google.com
Fri Aug 15 12:13:59 PDT 2014
A first round of comments.
================
Comment at: clang-highlight/ClangHighlight.cpp:43
@@ +42,3 @@
+
+static cl::opt<OutputFormat> OutputFormatFlag(
+ cl::desc("Output format for the highlighted code."),
----------------
Seems weird that only this one flag has the "Flag" suffix. Be consistent.
================
Comment at: clang-highlight/Fuzzy/AnnotatedToken.h:22
@@ +21,3 @@
+class AnnotatedToken {
+ Token Tok_;
+ ASTElement *Annot;
----------------
We generally don't use trailing "_" in Clang.
================
Comment at: clang-highlight/Fuzzy/AnnotatedToken.h:23
@@ +22,3 @@
+ Token Tok_;
+ ASTElement *Annot;
+
----------------
"Annot" doesn't seem like a good name. What does this contain?
================
Comment at: clang-highlight/Fuzzy/AnnotatedToken.h:35
@@ +34,3 @@
+
+ Token& Tok() { return Tok_; }
+ const Token& Tok() const { return Tok_; }
----------------
As both of these accessors are provided, wouldn't it make more sense to just make the member public?
================
Comment at: clang-highlight/Fuzzy/AnnotatedToken.h:40
@@ +39,3 @@
+ const ASTElement *getASTReference() const { return Annot; }
+ ASTElement *getASTReference() { return Annot; }
+ bool hasASTReference() const { return Annot; }
----------------
Is this ever modified? Can we make the member const?
================
Comment at: clang-highlight/Fuzzy/AnnotatedToken.h:44
@@ +43,3 @@
+
+class AnnotatedTokenRef {
+ AnnotatedToken *ATok;
----------------
Please provide a short comment on all classes describing what they are used for.
Why do you prefer this wrapper class over just handling AnnotatedToken pointers?
================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:34
@@ +33,3 @@
+ enum ASTElementClass {
+ NoASTElementClass = 0,
+ UnparsableBlockClass,
----------------
The convention in LLVM/Clang would be something like:
AEC_NoClass,
AEC_UnparsableBlock,
AEC_Type,
etc.
================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:76
@@ +75,3 @@
+private:
+ ASTElementClass sClass;
+};
----------------
Why "s"? Also, as per naming convention, this should start with upper case.
================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:80
@@ +79,3 @@
+/// An expression in it's classical sense. If an expression is used as a
+/// statement, it has to be embedded into a ExprStmt (yet to be implemented).
+/// Rationale is that there is otherwise no way to store the semicolon.
----------------
nit: .. an ExprStmt ..
================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:96
@@ +95,3 @@
+
+class TypeOrExpression {
+ std::unique_ptr<ASTElement> Ptr;
----------------
Do you gain much by using this class for template arguments (as opposed to just using ASTElement)?
================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:97
@@ +96,3 @@
+class TypeOrExpression {
+ std::unique_ptr<ASTElement> Ptr;
+
----------------
Ptr to what?
================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:122
@@ +121,3 @@
+ llvm::SmallVector<AnnotatedTokenRef, 1> NameSegments;
+ llvm::Optional<std::shared_ptr<TemplateArguments> > TemplateArgs;
+
----------------
I am quite strongly against the use of shared_ptr here. I understand that ownership in such a tree is somewhat difficult. How about doing what Clang itself does (storing all elements in an ASTContext and not worrying about ownership at all)?
================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:124
@@ +123,3 @@
+
+ void reown(ASTElement *Ref) {
+ for (auto &N : NameSegments)
----------------
As above, I think we should try to simplify ownership and just put all the nodes into a common context.
================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:158
@@ +157,3 @@
+ };
+ AnnotatedTokenRef Parens[END_EXPR];
+
----------------
Why not just two members LeftParen and RightParen?
The question also applies to all other node types. Do you expect to iterate over them somewhere?
================
Comment at: clang-highlight/Fuzzy/FuzzyASTPrinter.cpp:1
@@ +1,2 @@
+//===--- FuzzyParser.cpp - clang-highlight ----------------------*- C++ -*-===//
+//
----------------
The "*- C++ -*" is only useful in .h files.
================
Comment at: clang-highlight/Fuzzy/FuzzyASTPrinter.cpp:20
@@ +19,3 @@
+namespace {
+struct Indented {
+ const int Indent;
----------------
Take a look at how ASTDumper.cpp handles indents. IMO, the usage of a ScopedIndent seems easier to follow and maintain.
================
Comment at: clang-highlight/Fuzzy/FuzzyParser.cpp:21
@@ +20,3 @@
+namespace {
+template <bool SkipPreprocessor> class BasicTokenFilter {
+ AnnotatedToken *First, *Last;
----------------
Why is SkipPreprocessor a template argument (as opposed to a constructor parameter and member)?
================
Comment at: clang-highlight/Fuzzy/FuzzyParser.cpp:24
@@ +23,3 @@
+
+ void skipWhitespaces() {
+ for (;;) {
----------------
Why not just use Lexer::SetKeepWhitespaceMode?
================
Comment at: clang-highlight/Fuzzy/FuzzyParser.cpp:106
@@ +105,3 @@
+
+static int PrecedenceUnaryOperator = prec::PointerToMember + 1;
+static int PrecedenceArrowAndPeriod = prec::PointerToMember + 2;
----------------
Seems like unary operators would have lower precedence than pointers to members, no?
================
Comment at: clang-highlight/Fuzzy/FuzzyParser.cpp:107
@@ +106,3 @@
+static int PrecedenceUnaryOperator = prec::PointerToMember + 1;
+static int PrecedenceArrowAndPeriod = prec::PointerToMember + 2;
+
----------------
Are you sure that arrows and periods don't have the same precedence as pointers-to-members?
================
Comment at: clang-highlight/Fuzzy/FuzzyParser.cpp:109
@@ +108,3 @@
+
+static std::unique_ptr<Expr> parseExpr(TokenFilter &TF, int Precedence = 1,
+ bool StopAtGreater = false);
----------------
Why end the unnamed namespace above and make this and the following functions all static?
================
Comment at: clang-highlight/OutputWriter.cpp:35
@@ +34,3 @@
+
+static StdoutFormatInfo getFormatInfo(TokenClass Class) {
+ switch (Class) {
----------------
Same as above. Why end the unnamed namespace and then make these static?
================
Comment at: clang-highlight/OutputWriter.h:20
@@ +19,3 @@
+enum class OutputFormat {
+ StdoutColored,
+ HTML,
----------------
As per coding conventions, these should probably be:
OF_StdoutColored,
OF_HTML,
...
================
Comment at: clang-highlight/OutputWriter.h:28
@@ +27,3 @@
+enum class TokenClass {
+ NONE,
+ Type,
----------------
Same as above:
TC_None, // I don't see a reason for all-caps
TC_Type,
..
================
Comment at: clang-highlight/OutputWriter.h:49
@@ +48,3 @@
+
+// \brief Creates a output writer that writes in the specified Format to stdout
+std::unique_ptr<OutputWriter> makeOutputWriter(OutputFormat Format,
----------------
\brief only makes sense inside doxygen comments (///)
================
Comment at: clang-highlight/TokenClassifier.cpp:39
@@ +38,3 @@
+
+bool isCharLiteral(tok::TokenKind TK) {
+ switch (TK) {
----------------
Seems like this should be in the same file as isDeclSpecifier, etc.
================
Comment at: clang-highlight/TokenClassifier.cpp:176
@@ +175,3 @@
+ }
+ if (isa<fuzzy::PPString>(R)) {
+ Class = TokenClass::String;
----------------
In LLVM/Clang, we generally don't put braces around such trivial one-statement ifs. Here and everywhere else.
================
Comment at: clang-highlight/TokenClassifier.h:25
@@ +24,3 @@
+
+void highlight(std::unique_ptr<llvm::MemoryBuffer> Source,
+ llvm::StringRef FileName, std::unique_ptr<OutputWriter> OW,
----------------
Should you be passing unique_ptrs for MemoryBuffer and OutputWriter here? That would suggest to me that highlight() takes ownership. Is that really a good idea / necessary?
================
Comment at: clang-highlight/unittests/FuzzyParseTest.cpp:45
@@ +44,3 @@
+
+LangOptions getFormattingLangOpts(bool Cpp03 = false) {
+ LangOptions LangOpts;
----------------
"Formatting"?
================
Comment at: clang-highlight/unittests/FuzzyParseTest.cpp:190
@@ +189,3 @@
+ dump(Parsed, Code);
+ EXPECT_TRUE(Parsed.TU.children().size() > 0);
+ return;
----------------
How can this ever be true?
http://reviews.llvm.org/D4725
More information about the cfe-commits
mailing list