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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
