[llvm] address more comments

Author: PeterChou1

llvm/include/llvm/Support/Mustache.h

@@ -130,7 +130,7 @@ class ASTNode {
     InvertSection,
   };
 
-  ASTNode() : T(Type::Root), ParentContext(nullptr), LocalContext(nullptr) {};
+  ASTNode() : T(Type::Root), ParentContext(nullptr) {};
 
   ASTNode(StringRef Body, ASTNode *Parent)
       : T(Type::Text), Body(Body), Parent(Parent), ParentContext(nullptr),
@@ -151,12 +151,12 @@ class ASTNode {
 
   void render(const llvm::json::Value &Data, SmallString<0> &Output);
 
-  void setUpNode(StringMap<ASTNode *> &Partials, StringMap<Lambda> &Lambdas,
+  void setUpNode(llvm::BumpPtrAllocator &Alloc,
+                 StringMap<ASTNode *> &Partials, 
+                 StringMap<Lambda> &Lambdas,
                  StringMap<SectionLambda> &SectionLambdas,
                  DenseMap<char, StringRef> &Escapes);
-
-  void setUpContext(llvm::BumpPtrAllocator *Alloc);
-
+  
 private:
 
   void renderLambdas(const llvm::json::Value &Contexts, SmallString<0> &Output,
@@ -212,9 +212,12 @@ class Template {
   StringMap<Lambda> Lambdas;
   StringMap<SectionLambda> SectionLambdas;
   DenseMap<char, StringRef> Escapes;
+  // The allocator for the ASTNode Tree
   llvm::BumpPtrAllocator Allocator;
+  // Allocator for each render call resets after each render
+  llvm::BumpPtrAllocator LocalAllocator;
   ASTNode *Tree;
 };
 } // namespace mustache
 } // end namespace llvm
-#endif // LLVM_SUPPORT_MUSTACHE
+#endif // LLVM_SUPPORT_MUSTACHE

llvm/lib/Support/Mustache.cpp

@@ -16,8 +16,8 @@ using namespace llvm::mustache;
 
 SmallString<0> escapeString(StringRef Input,
                             DenseMap<char, StringRef> &Escape) {
-
   SmallString<0> Output;
+  Output.reserve(Input.size());
   for (char C : Input) {
     auto It = Escape.find(C);
     if (It != Escape.end())
@@ -91,24 +91,33 @@ Token::Type Token::getTokenType(char Identifier) {
 
 // Function to check if there's no meaningful text behind
 bool noTextBehind(size_t Idx, const SmallVector<Token, 0> &Tokens) {
-  if (Idx == 0 || Tokens[Idx - 1].getType() != Token::Type::Text)
+  if(Idx == 0)
+    return false;
+  
+  int PrevIdx = Idx - 1;
+  if (Tokens[PrevIdx].getType() != Token::Type::Text)
     return false;
+  
   const Token &PrevToken = Tokens[Idx - 1];
   StringRef TokenBody = PrevToken.getRawBody().rtrim(" \t\v\t");
-  // We make a special exception for when previous token is empty
+  // We make an exception for when previous token is empty
   // and the current token is the second token
   // ex.
   //  " {{#section}}A{{/section}}"
-  // that's why we check if the token body is empty and the index is 1
+  // the output of this is
+  //  "A"
   return TokenBody.ends_with("\n") || (TokenBody.empty() && Idx == 1);
 }
 
 // Function to check if there's no meaningful text ahead
 bool noTextAhead(size_t Idx, const SmallVector<Token, 0> &Tokens) {
-  if (Idx >= Tokens.size() - 1 ||
-      Tokens[Idx + 1].getType() != Token::Type::Text)
+  if (Idx >= Tokens.size() - 1)
     return false;
-
+  
+  int PrevIdx = Idx + 1;
+  if (Tokens[Idx + 1].getType() != Token::Type::Text)
+    return false;
+  
   const Token &NextToken = Tokens[Idx + 1];
   StringRef TokenBody = NextToken.getRawBody().ltrim(" ");
   return TokenBody.starts_with("\r\n") || TokenBody.starts_with("\n");
@@ -121,10 +130,47 @@ bool requiresCleanUp(Token::Type T) {
          T == Token::Type::Partial;
 }
 
-// Simple tokenizer that splits the template into tokens
-// the mustache spec allows {{{ }}} to unescape variables
-// but we don't support that here unescape variable
-// is represented only by {{& variable}}
+// Adjust next token body if there is no text ahead
+// eg.
+//  The template string
+//  "{{! Comment }} \nLine 2"
+// would be considered as no text ahead and should be render as
+//  " Line 2"
+void stripTokenAhead(SmallVector<Token, 0>& Tokens, size_t Idx) {
+  Token &NextToken = Tokens[Idx + 1];
+  StringRef NextTokenBody = NextToken.getTokenBody();
+  // cut off the leading newline which could be \n or \r\n
+  if (NextTokenBody.starts_with("\r\n"))
+    NextToken.setTokenBody(NextTokenBody.substr(2));
+  else if (NextTokenBody.starts_with("\n"))
+    NextToken.setTokenBody(NextTokenBody.substr(1));
+}
+
+// Adjust previous token body if there no text behind
+// eg.
+//  The template string
+//  " \t{{#section}}A{{/section}}"
+// would be considered as no text ahead and should be render as
+//  "A"
+// The exception for this is partial tag which requires us to
+// keep track of the indentation once it's rendered
+void stripTokenBefore(SmallVector<Token, 0>& Tokens, 
+                      size_t Idx,
+                      Token& CurrentToken,
+                      Token::Type CurrentType) {
+  Token &PrevToken = Tokens[Idx - 1];
+  StringRef PrevTokenBody = PrevToken.getTokenBody();
+  StringRef Unindented = PrevTokenBody.rtrim(" \t\v");
+  size_t Indentation = PrevTokenBody.size() - Unindented.size();
+  if (CurrentType != Token::Type::Partial)
+    PrevToken.setTokenBody(Unindented);
+  CurrentToken.setIndentation(Indentation);
+}
+
+// Simple tokenizer that splits the template into tokens.
+// The mustache spec allows {{{ }}} to unescape variables
+// but we don't support that here. An unescape variable
+// is represented only by {{& variable}}.
 SmallVector<Token, 0> tokenize(StringRef Template) {
   SmallVector<Token, 0> Tokens;
   StringRef Open("{{");
@@ -160,7 +206,11 @@ SmallVector<Token, 0> tokenize(StringRef Template) {
     Tokens.emplace_back(Template.substr(Start));
 
   // Fix up white spaces for:
-  //  open sections/inverted sections/close section/comment
+  //   - open sections
+  //   - inverted sections
+  //   - close sections
+  //   - comments
+  //
   // This loop attempts to find standalone tokens and tries to trim out
   // the surrounding whitespace.
   // For example:
@@ -178,47 +228,22 @@ SmallVector<Token, 0> tokenize(StringRef Template) {
     if (!RequiresCleanUp)
       continue;
 
-    // We adjust the token body if there's no text behind or ahead a token is
-    // considered surrounded by no text if the right of the previous token
-    // is a newline followed by spaces or if the left of the next token
-    // is spaces followed by a newline
+    // We adjust the token body if there's no text behind or ahead. 
+    // A token is considered to have no text ahead if the right of the previous
+    // token is a newline followed by spaces.
+    // A token is considered to have no text behind if the left of the next 
+    // token is spaces followed by a newline.
     // eg.
     //  "Line 1\n {{#section}} \n Line 2 \n {{/section}} \n Line 3"
-
     bool NoTextBehind = noTextBehind(Idx, Tokens);
     bool NoTextAhead = noTextAhead(Idx, Tokens);
-
-    // Adjust next token body if there is no text ahead
-    // eg.
-    //  The template string
-    //  "{{! Comment }} \nLine 2"
-    // would be considered as no text ahead and should be render as
-    //  " Line 2"
+    
     if ((NoTextBehind && NoTextAhead) || (NoTextAhead && Idx == 0)) {
-      Token &NextToken = Tokens[Idx + 1];
-      StringRef NextTokenBody = NextToken.getTokenBody();
-      // cut off the leading newline which could be \n or \r\n
-      if (NextTokenBody.starts_with("\r\n"))
-        NextToken.setTokenBody(NextTokenBody.substr(2));
-      else if (NextTokenBody.starts_with("\n"))
-        NextToken.setTokenBody(NextTokenBody.substr(1));
+      stripTokenAhead(Tokens, Idx);
     }
-    // Adjust previous token body if there no text behind
-    // eg.
-    //  The template string
-    //  " \t{{#section}}A{{/section}}"
-    // would be considered as no text ahead and should be render as
-    //  "A"
-    // The exception for this is partial tag which requires us to
-    // keep track of the indentation once it's rendered
+    
     if (((NoTextBehind && NoTextAhead) || (NoTextBehind && Idx == LastIdx))) {
-      Token &PrevToken = Tokens[Idx - 1];
-      StringRef PrevTokenBody = PrevToken.getTokenBody();
-      StringRef Unindented = PrevTokenBody.rtrim(" \t\v");
-      size_t Indentation = PrevTokenBody.size() - Unindented.size();
-      if (CurrentType != Token::Type::Partial)
-        PrevToken.setTokenBody(Unindented);
-      CurrentToken.setIndentation(Indentation);
+      stripTokenBefore(Tokens, Idx, CurrentToken, CurrentType);
     }
   }
   return Tokens;
@@ -304,25 +329,25 @@ void Parser::parseMustache(ASTNode *Parent) {
       Parent->addChild(CurrentNode);
       break;
     }
+    case Token::Type::Comment:
+      break;
     case Token::Type::SectionClose:
       return;
-    default:
-      break;
     }
   }
 }
 
 StringRef Template::render(Value &Data) {
-  BumpPtrAllocator LocalAllocator;
-  Tree->setUpContext(&LocalAllocator);
   Tree->render(Data, Output);
+  LocalAllocator.Reset();
   return Output.str();
 }
 
 void Template::registerPartial(StringRef Name, StringRef Partial) {
   Parser P = Parser(Partial, Allocator);
   ASTNode *PartialTree = P.parse();
-  PartialTree->setUpNode(Partials, Lambdas, SectionLambdas, Escapes);
+  PartialTree->setUpNode(LocalAllocator, Partials, Lambdas, SectionLambdas, 
+                         Escapes);
   Partials.insert(std::make_pair(Name, PartialTree));
 }
 
@@ -344,7 +369,8 @@ Template::Template(StringRef TemplateStr) {
                                             {'"', "&quot;"},
                                             {'\'', "&#39;"}};
   registerEscape(HtmlEntities);
-  Tree->setUpNode(Partials, Lambdas, SectionLambdas, Escapes);
+  Tree->setUpNode(LocalAllocator,
+                  Partials, Lambdas, SectionLambdas, Escapes);
 }
 
 void toJsonString(const Value &Data, SmallString<0> &Output) {
@@ -494,7 +520,6 @@ void ASTNode::renderChild(const Value &Contexts, SmallString<0> &Output) {
 
 void ASTNode::renderPartial(const Value &Contexts, SmallString<0> &Output,
                             ASTNode *Partial) {
-  Partial->setUpContext(Allocator);
   Partial->render(Contexts, Output);
   addIndentation(Output, Indentation);
 }
@@ -506,8 +531,8 @@ void ASTNode::renderLambdas(const Value &Contexts, SmallString<0> &Output,
   toJsonString(LambdaResult, LambdaStr);
   Parser P = Parser(LambdaStr, *Allocator);
   ASTNode *LambdaNode = P.parse();
-  LambdaNode->setUpNode(*Partials, *Lambdas, *SectionLambdas, *Escapes);
-  LambdaNode->setUpContext(Allocator);
+  LambdaNode->setUpNode(*Allocator, *Partials, *Lambdas, *SectionLambdas, 
+                        *Escapes);
   LambdaNode->render(Contexts, Output);
   if (T == Variable)
     Output = escapeString(Output, *Escapes);
@@ -523,30 +548,24 @@ void ASTNode::renderSectionLambdas(const Value &Contexts,
   toJsonString(Return, LambdaStr);
   Parser P = Parser(LambdaStr, *Allocator);
   ASTNode *LambdaNode = P.parse();
-  LambdaNode->setUpNode(*Partials, *Lambdas, *SectionLambdas, *Escapes);
-  LambdaNode->setUpContext(Allocator);
+  LambdaNode->setUpNode(*Allocator, *Partials, *Lambdas, *SectionLambdas, 
+                        *Escapes);
   LambdaNode->render(Contexts, Output);
   return;
 }
 
-void ASTNode::setUpNode(StringMap<ASTNode *> &Par, StringMap<Lambda> &L,
+void ASTNode::setUpNode(llvm::BumpPtrAllocator &Alloc,
+                        StringMap<ASTNode *> &Par, StringMap<Lambda> &L,
                         StringMap<SectionLambda> &SC,
                         DenseMap<char, StringRef> &E) {
   // Passed down datastructures needed for rendering to
   // the children nodes. This must be called before rendering
+  Allocator = &Alloc;
   Partials = &Par;
   Lambdas = &L;
   SectionLambdas = &SC;
   Escapes = &E;
   for (ASTNode *Child : Children)
-    Child->setUpNode(Par, L, SC, E);
+    Child->setUpNode(Alloc, Par, L, SC, E);
 }
 
-void ASTNode::setUpContext(llvm::BumpPtrAllocator *Alloc) {
-  // Passed down the allocator to the children nodes.
-  // Each render call requires a allocator for generating
-  // Partial/Section nodes
-  Allocator = Alloc;
-  for (ASTNode *Child : Children)
-    Child->setUpContext(Alloc);
-}