================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:122
@@ +121,3 @@
+ llvm::SmallVector<AnnotatedTokenRef, 1> NameSegments;
+ llvm::Optional<std::shared_ptr<TemplateArguments> > TemplateArgs;
+
----------------
kapf wrote:
> djasper 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.
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.
================
Comment at: clang-highlight/Fuzzy/FuzzyAST.h:124
@@ +123,3 @@
+
+ void reown(ASTElement *Ref) {
+ for (auto &N : NameSegments)
----------------
kapf wrote:
> djasper 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.
I don't think you need this kind of ownership consideration if you follow the
advise above.
================
Comment at: clang-highlight/Fuzzy/FuzzyParser.cpp:24
@@ +23,3 @@
+
+ void skipWhitespaces() {
+ for (;;) {
----------------
kapf wrote:
> djasper wrote:
> > Why not just use Lexer::SetKeepWhitespaceMode?
> Because this also skips comments which need to be highlighted.
Even if you also use SetCommentRetentionState()?
================
Comment at: clang-highlight/Fuzzy/FuzzyParser.cpp:109
@@ +108,3 @@
+
+static std::unique_ptr<Expr> parseExpr(TokenFilter &TF, int Precedence = 1,
+ bool StopAtGreater = false);
----------------
kapf wrote:
> djasper 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.
No. Coding guidelines are the law ;-)..
http://reviews.llvm.org/D4725
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
