[PATCH] Add new tool clang-highlight to clang/tools/extra

[Johannes Kapfhammer]

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

================
Comment at: clang-highlight/Fuzzy/AnnotatedToken.h:35
@@ +34,3 @@
+
+  Token& Tok() { return Tok_; }
+  const Token& Tok() const { return Tok_; }
----------------
Daniel Jasper wrote:
> As both of these accessors are provided, wouldn't it make more sense to just make the member public?
I will just make the members public then, this should solve the naming issues as well.

================
Comment at: clang-highlight/Fuzzy/AnnotatedToken.h:44
@@ +43,3 @@
+
+class AnnotatedTokenRef {
+  AnnotatedToken *ATok;
----------------
Daniel Jasper wrote:
> 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?
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; }
```
.

================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:34
@@ +33,3 @@
+  enum ASTElementClass {
+    NoASTElementClass = 0,
+    UnparsableBlockClass,
----------------
Daniel Jasper wrote:
> The convention in LLVM/Clang would be something like:
> 
>   AEC_NoClass,
>   AEC_UnparsableBlock,
>   AEC_Type,
> 
> etc.
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.

================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:122
@@ +121,3 @@
+  llvm::SmallVector<AnnotatedTokenRef, 1> NameSegments;
+  llvm::Optional<std::shared_ptr<TemplateArguments> > TemplateArgs;
+
----------------
Daniel Jasper wrote:
> 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)?
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.

================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:124
@@ +123,3 @@
+
+  void reown(ASTElement *Ref) {
+    for (auto &N : NameSegments)
----------------
Daniel Jasper wrote:
> As above, I think we should try to simplify ownership and just put all the nodes into a common context.
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.

================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:158
@@ +157,3 @@
+  };
+  AnnotatedTokenRef Parens[END_EXPR];
+
----------------
Daniel Jasper wrote:
> 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?
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.

================
Comment at: clang-highlight/Fuzzy/FuzzyParser.cpp:24
@@ +23,3 @@
+
+  void skipWhitespaces() {
+    for (;;) {
----------------
Daniel Jasper wrote:
> Why not just use Lexer::SetKeepWhitespaceMode?
Because this also skips comments which need to be highlighted.

================
Comment at: clang-highlight/Fuzzy/FuzzyParser.cpp:109
@@ +108,3 @@
+
+static std::unique_ptr<Expr> parseExpr(TokenFilter &TF, int Precedence = 1,
+                                       bool StopAtGreater = false);
----------------
Daniel Jasper wrote:
> Why end the unnamed namespace above and make this and the following functions all static?
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.

================
Comment at: clang-highlight/OutputWriter.h:20
@@ +19,3 @@
+enum class OutputFormat {
+  StdoutColored,
+  HTML,
----------------
Daniel Jasper wrote:
> As per coding conventions, these should probably be:
> 
>   OF_StdoutColored,
>   OF_HTML,
>   ...
Redundant because of "enum class", but I can change the enum class back to a plain enum and add the OF_ prefix if wished.

http://reviews.llvm.org/D4725