This is an archive of the discontinued LLVM Phabricator instance.

Add new tool clang-highlight to clang/tools/extra
Needs ReviewPublic

Authored by kapf on Jul 30 2014, 9:16 AM.

Details

Reviewers
djasper
Summary

Initial commit for a standalone tool that highlights C++ code snippets to various output formats.

This revision consists of a general fuzzy parser library (extra/clang-highlight/Fuzzy), a command line frontend clang-highlight (extra/clang-highlight), tests (extra/clang-highlight/unittests) and a small LaTeX package (extra/clang-highlight/latex). Refer to the Documentation (extra/docs) for more details.

Development has taken place on https://github.com/kapf/clang-highlight.

Diff Detail

Event Timeline

kapf updated this revision to Diff 12034.Jul 30 2014, 9:16 AM
kapf retitled this revision from to Add new tool clang-highlight to clang/tools/extra.
kapf updated this object.
kapf edited the test plan for this revision. (Show Details)
kapf added a reviewer: djasper.
kapf added a subscriber: Unknown Object (MLST).
djasper edited edge metadata.Aug 15 2014, 12:13 PM

A first round of comments.

clang-highlight/ClangHighlight.cpp
43

Seems weird that only this one flag has the "Flag" suffix. Be consistent.

clang-highlight/Fuzzy/AnnotatedToken.h
22

We generally don't use trailing "_" in Clang.

23

"Annot" doesn't seem like a good name. What does this contain?

35

As both of these accessors are provided, wouldn't it make more sense to just make the member public?

40

Is this ever modified? Can we make the member const?

44

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?

clang-highlight/Fuzzy/FuzzyAST.h
34

The convention in LLVM/Clang would be something like:

AEC_NoClass,
AEC_UnparsableBlock,
AEC_Type,

etc.

76

Why "s"? Also, as per naming convention, this should start with upper case.

80

nit: .. an ExprStmt ..

96

Do you gain much by using this class for template arguments (as opposed to just using ASTElement)?

97

Ptr to what?

122

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)?

124

As above, I think we should try to simplify ownership and just put all the nodes into a common context.

158

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?

clang-highlight/Fuzzy/FuzzyASTPrinter.cpp
1

The "*- C++ -*" is only useful in .h files.

20

Take a look at how ASTDumper.cpp handles indents. IMO, the usage of a ScopedIndent seems easier to follow and maintain.

clang-highlight/Fuzzy/FuzzyParser.cpp
21

Why is SkipPreprocessor a template argument (as opposed to a constructor parameter and member)?

24

Why not just use Lexer::SetKeepWhitespaceMode?

106

Seems like unary operators would have lower precedence than pointers to members, no?

107

Are you sure that arrows and periods don't have the same precedence as pointers-to-members?

109

Why end the unnamed namespace above and make this and the following functions all static?

clang-highlight/OutputWriter.cpp
35

Same as above. Why end the unnamed namespace and then make these static?

clang-highlight/OutputWriter.h
20

As per coding conventions, these should probably be:

OF_StdoutColored,
OF_HTML,
...
28

Same as above:

TC_None, // I don't see a reason for all-caps
TC_Type,
..
49

\brief only makes sense inside doxygen comments (///)

clang-highlight/TokenClassifier.cpp
39

Seems like this should be in the same file as isDeclSpecifier, etc.

176

In LLVM/Clang, we generally don't put braces around such trivial one-statement ifs. Here and everywhere else.

clang-highlight/TokenClassifier.h
25

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?

clang-highlight/unittests/FuzzyParseTest.cpp
45

"Formatting"?

190

How can this ever be true?

kapf added a comment.Aug 15 2014, 1:42 PM

Agree with most comments, will upload a fix later. Replies inline.

clang-highlight/Fuzzy/AnnotatedToken.h
35

I will just make the members public then, this should solve the naming issues as well.

44

Just syntactic sugar, really. It enforces to set the AST reference or explicitly not. I find the usage

MyASTNode : Tok(Tok, this) {}

nicer than

MyASTNode : Tok(Tok) { Tok->ASTReference = this; }

.

clang-highlight/Fuzzy/FuzzyAST.h
34

This is the naming I found in Clang (Stmt::DeclRefExprClass). As this enum is inside the class ASTElement, the user would write ASTElement::UnparsableBlockClass, not just UnparsableBlock. ASTElement::AEC_UnparsableBlock seems redundant to me.

122

I thought not using an ASTContext would be simpler, but it might not be simpler in the end. The two alternatives are ASTContext or a clone method. I agree that ASTContext would be the cleanest solution.

124

This is a different ownership. Every token points to the AST and every AST node "owns" a list of tokens. When a QualifiedID is parsed, it isn't decided yet whether it'll be part of a Type or a CallExpr. This reown handles the ast reference and cannot be simplified by an ASTContext.

158

This is the approach found in clang/AST/Stmt.h. It simplifies the setters by introducing a "setRef" method (useful by >2 entries). Setters are needed because they also set the AST reference of the token.

clang-highlight/Fuzzy/FuzzyParser.cpp
24

Because this also skips comments which need to be highlighted.

109

I followed the LLVM guideline: http://llvm.org/docs/CodingStandards.html#anonymous-namespaces
I don't quite agree with the reasoning, so I'm happy to change this.

clang-highlight/OutputWriter.h
20

Redundant because of "enum class", but I can change the enum class back to a plain enum and add the OF_ prefix if wished.

djasper added inline comments.Feb 4 2015, 6:12 AM
clang-highlight/Fuzzy/FuzzyAST.h
122

I don't mean ASTContext specifically, but create a storage object which simply owns all objects. Then you'll only need to handle pointers later.

124

I don't think you need this kind of ownership consideration if you follow the advise above.

clang-highlight/Fuzzy/FuzzyParser.cpp
24

Even if you also use SetCommentRetentionState()?

109

No. Coding guidelines are the law ;-)..