- 
                Notifications
    You must be signed in to change notification settings 
- Fork 15k
[llvm][mustache] Simplify debug logging #159193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| @llvm/pr-subscribers-llvm-support Author: Paul Kirth (ilovepi) ChangesFull diff: https://github.com/llvm/llvm-project/pull/159193.diff 1 Files Affected: 
 diff --git a/llvm/lib/Support/Mustache.cpp b/llvm/lib/Support/Mustache.cpp
index e084e379312ef..6dab658ea535d 100644
--- a/llvm/lib/Support/Mustache.cpp
+++ b/llvm/lib/Support/Mustache.cpp
@@ -187,14 +187,14 @@ class ASTNode {
   void render(const llvm::json::Value &Data, MustacheOutputStream &OS);
 
 private:
-  void renderLambdas(const llvm::json::Value &Contexts, MustacheOutputStream &OS,
-                     Lambda &L);
+  void renderLambdas(const llvm::json::Value &Contexts,
+                     MustacheOutputStream &OS, Lambda &L);
 
   void renderSectionLambdas(const llvm::json::Value &Contexts,
                             MustacheOutputStream &OS, SectionLambda &L);
 
-  void renderPartial(const llvm::json::Value &Contexts, MustacheOutputStream &OS,
-                     ASTNode *Partial);
+  void renderPartial(const llvm::json::Value &Contexts,
+                     MustacheOutputStream &OS, ASTNode *Partial);
 
   void renderChild(const llvm::json::Value &Context, MustacheOutputStream &OS);
 
@@ -204,9 +204,11 @@ class ASTNode {
   void renderText(MustacheOutputStream &OS);
   void renderPartial(const json::Value &CurrentCtx, MustacheOutputStream &OS);
   void renderVariable(const json::Value &CurrentCtx, MustacheOutputStream &OS);
-  void renderUnescapeVariable(const json::Value &CurrentCtx, MustacheOutputStream &OS);
+  void renderUnescapeVariable(const json::Value &CurrentCtx,
+                              MustacheOutputStream &OS);
   void renderSection(const json::Value &CurrentCtx, MustacheOutputStream &OS);
-  void renderInvertSection(const json::Value &CurrentCtx, MustacheOutputStream &OS);
+  void renderInvertSection(const json::Value &CurrentCtx,
+                           MustacheOutputStream &OS);
 
   MustacheContext &Ctx;
   Type Ty;
@@ -330,6 +332,36 @@ struct Tag {
   size_t StartPosition = StringRef::npos;
 };
 
+static const char *tagKindToString(Tag::Kind K) {
+  switch (K) {
+  case Tag::Kind::None:
+    return "None";
+  case Tag::Kind::Normal:
+    return "Normal";
+  case Tag::Kind::Triple:
+    return "Triple";
+  }
+  llvm_unreachable("Unknown Tag::Kind");
+}
+
+static const char *jsonKindToString(json::Value::Kind K) {
+  switch (K) {
+  case json::Value::Kind::Null:
+    return "JSON_KIND_NULL";
+  case json::Value::Kind::Boolean:
+    return "JSON_KIND_BOOLEAN";
+  case json::Value::Kind::Number:
+    return "JSON_KIND_NUMBER";
+  case json::Value::Kind::String:
+    return "JSON_KIND_STRING";
+  case json::Value::Kind::Array:
+    return "JSON_KIND_ARRAY";
+  case json::Value::Kind::Object:
+    return "JSON_KIND_OBJECT";
+  }
+  llvm_unreachable("Unknown json::Value::Kind");
+}
+
 static Tag findNextTag(StringRef Template, size_t StartPos, StringRef Open,
                        StringRef Close) {
   const StringLiteral TripleOpen("{{{");
@@ -374,11 +406,10 @@ static Tag findNextTag(StringRef Template, size_t StartPos, StringRef Open,
 
 static std::optional<std::pair<StringRef, StringRef>>
 processTag(const Tag &T, SmallVectorImpl<Token> &Tokens) {
-  LLVM_DEBUG(dbgs() << "  Found tag: \"" << T.FullMatch << "\", Content: \""
-                    << T.Content << "\"\n");
+  LLVM_DEBUG(dbgs() << "[Tag] " << T.FullMatch << ", Content: " << T.Content
+                    << ", Kind: " << tagKindToString(T.TagKind) << "\n");
   if (T.TagKind == Tag::Kind::Triple) {
     Tokens.emplace_back(T.FullMatch.str(), "&" + T.Content.str(), '&');
-    LLVM_DEBUG(dbgs() << "  Created UnescapeVariable token.\n");
     return std::nullopt;
   }
   StringRef Interpolated = T.Content;
@@ -386,7 +417,6 @@ processTag(const Tag &T, SmallVectorImpl<Token> &Tokens) {
   if (!Interpolated.trim().starts_with("=")) {
     char Front = Interpolated.empty() ? ' ' : Interpolated.trim().front();
     Tokens.emplace_back(RawBody, Interpolated.str(), Front);
-    LLVM_DEBUG(dbgs() << "  Created tag token of type '" << Front << "'\n");
     return std::nullopt;
   }
   Tokens.emplace_back(RawBody, Interpolated.str(), '=');
@@ -396,8 +426,8 @@ processTag(const Tag &T, SmallVectorImpl<Token> &Tokens) {
   DelimSpec = DelimSpec.trim();
 
   auto [NewOpen, NewClose] = DelimSpec.split(' ');
-  LLVM_DEBUG(dbgs() << "  Found Set Delimiter tag. NewOpen='" << NewOpen
-                    << "', NewClose='" << NewClose << "'\n");
+  LLVM_DEBUG(dbgs() << "[Set Delimiter] NewOpen: " << NewOpen
+                    << ", NewClose: " << NewClose << "\n");
   return std::make_pair(NewOpen, NewClose);
 }
 
@@ -406,22 +436,20 @@ processTag(const Tag &T, SmallVectorImpl<Token> &Tokens) {
 // but we don't support that here. An unescape variable
 // is represented only by {{& variable}}.
 static SmallVector<Token> tokenize(StringRef Template) {
-  LLVM_DEBUG(dbgs() << "Tokenizing template: \"" << Template << "\"\n");
+  LLVM_DEBUG(dbgs() << "[Tokenize Template] \"" << Template << "\"\n");
   SmallVector<Token> Tokens;
   SmallString<8> Open("{{");
   SmallString<8> Close("}}");
   size_t Start = 0;
 
   while (Start < Template.size()) {
-    LLVM_DEBUG(dbgs() << "Loop start. Start=" << Start << ", Open='" << Open
+    LLVM_DEBUG(dbgs() << "[Tokenize Loop] Start=" << Start << ", Open='" << Open
                       << "', Close='" << Close << "'\n");
     Tag T = findNextTag(Template, Start, Open, Close);
 
     if (T.TagKind == Tag::Kind::None) {
       // No more tags, the rest is text.
       Tokens.emplace_back(Template.substr(Start).str());
-      LLVM_DEBUG(dbgs() << "  No more tags. Created final Text token: \"" 
-                        << Template.substr(Start) << "\"\n");
       break;
     }
 
@@ -429,7 +457,6 @@ static SmallVector<Token> tokenize(StringRef Template) {
     if (T.StartPosition > Start) {
       StringRef Text = Template.substr(Start, T.StartPosition - Start);
       Tokens.emplace_back(Text.str());
-      LLVM_DEBUG(dbgs() << "  Created Text token: \"" << Text << "\"\n");
     }
 
     if (auto NewDelims = processTag(T, Tokens)) {
@@ -480,15 +507,13 @@ static SmallVector<Token> tokenize(StringRef Template) {
     if ((!HasTextBehind && !HasTextAhead) || (!HasTextBehind && Idx == LastIdx))
       stripTokenBefore(Tokens, Idx, CurrentToken, CurrentType);
   }
-  LLVM_DEBUG(dbgs() << "Tokenizing finished.\n");
   return Tokens;
 }
 
 // Custom stream to escape strings.
 class EscapeStringStream : public MustacheOutputStream {
 public:
-  explicit EscapeStringStream(raw_ostream &WrappedStream,
-                              EscapeMap &Escape)
+  explicit EscapeStringStream(raw_ostream &WrappedStream, EscapeMap &Escape)
       : Escape(Escape), WrappedStream(WrappedStream) {
     SetUnbuffered();
   }
@@ -532,7 +557,7 @@ class AddIndentationStringStream : public MustacheOutputStream {
     Indent.resize(Indentation, ' ');
 
     for (char C : Data) {
-      LLVM_DEBUG(dbgs() << "IndentationStream: NeedsIndent=" << NeedsIndent
+      LLVM_DEBUG(dbgs() << "[IndentationStream] NeedsIndent=" << NeedsIndent
                         << ", C='" << C << "', Indentation=" << Indentation
                         << "\n");
       if (NeedsIndent && C != '\n') {
@@ -641,7 +666,8 @@ void Parser::parseMustache(ASTNode *Parent) {
   }
 }
 static void toMustacheString(const json::Value &Data, raw_ostream &OS) {
-  LLVM_DEBUG(dbgs() << "toMustacheString: kind=" << (int)Data.kind() << "\n");
+  LLVM_DEBUG(dbgs() << "[To Mustache String] Kind: " << jsonKindToString(Data.kind())
+                    << ", Data: " << Data << "\n");
   switch (Data.kind()) {
   case json::Value::Null:
     return;
@@ -654,7 +680,6 @@ static void toMustacheString(const json::Value &Data, raw_ostream &OS) {
   }
   case json::Value::String: {
     auto Str = *Data.getAsString();
-    LLVM_DEBUG(dbgs() << "  --> writing string: \"" << Str << "\"\n");
     OS << Str.str();
     return;
   }
@@ -674,21 +699,24 @@ static void toMustacheString(const json::Value &Data, raw_ostream &OS) {
   }
 }
 
-void ASTNode::renderRoot(const json::Value &CurrentCtx, MustacheOutputStream &OS) {
+void ASTNode::renderRoot(const json::Value &CurrentCtx,
+                         MustacheOutputStream &OS) {
   renderChild(CurrentCtx, OS);
 }
 
 void ASTNode::renderText(MustacheOutputStream &OS) { OS << Body; }
 
-void ASTNode::renderPartial(const json::Value &CurrentCtx, MustacheOutputStream &OS) {
-  LLVM_DEBUG(dbgs() << "renderPartial: Accessor=" << AccessorValue[0]
+void ASTNode::renderPartial(const json::Value &CurrentCtx,
+                            MustacheOutputStream &OS) {
+  LLVM_DEBUG(dbgs() << "[Render Partial] Accessor=" << AccessorValue[0]
                     << ", Indentation=" << Indentation << "\n");
   auto Partial = Ctx.Partials.find(AccessorValue[0]);
   if (Partial != Ctx.Partials.end())
     renderPartial(CurrentCtx, OS, Partial->getValue().get());
 }
 
-void ASTNode::renderVariable(const json::Value &CurrentCtx, MustacheOutputStream &OS) {
+void ASTNode::renderVariable(const json::Value &CurrentCtx,
+                             MustacheOutputStream &OS) {
   auto Lambda = Ctx.Lambdas.find(AccessorValue[0]);
   if (Lambda != Ctx.Lambdas.end()) {
     renderLambdas(CurrentCtx, OS, Lambda->getValue());
@@ -700,13 +728,12 @@ void ASTNode::renderVariable(const json::Value &CurrentCtx, MustacheOutputStream
 
 void ASTNode::renderUnescapeVariable(const json::Value &CurrentCtx,
                                      MustacheOutputStream &OS) {
-  LLVM_DEBUG(dbgs() << "renderUnescapeVariable: Accessor=" << AccessorValue[0]
+  LLVM_DEBUG(dbgs() << "[Render UnescapeVariable] Accessor=" << AccessorValue[0]
                     << "\n");
   auto Lambda = Ctx.Lambdas.find(AccessorValue[0]);
   if (Lambda != Ctx.Lambdas.end()) {
     renderLambdas(CurrentCtx, OS, Lambda->getValue());
   } else if (const json::Value *ContextPtr = findContext()) {
-    LLVM_DEBUG(dbgs() << "  --> Found context value, writing to stream.\n");
     OS.suspendIndentation();
     toMustacheString(*ContextPtr, OS);
     OS.resumeIndentation();
@@ -714,7 +741,7 @@ void ASTNode::renderUnescapeVariable(const json::Value &CurrentCtx,
 }
 
 void ASTNode::renderSection(const json::Value &CurrentCtx,
-                              MustacheOutputStream &OS) {
+                            MustacheOutputStream &OS) {
   auto SectionLambda = Ctx.SectionLambdas.find(AccessorValue[0]);
   if (SectionLambda != Ctx.SectionLambdas.end()) {
     renderSectionLambdas(CurrentCtx, OS, SectionLambda->getValue());
@@ -776,8 +803,6 @@ void ASTNode::render(const llvm::json::Value &Data, MustacheOutputStream &OS) {
 }
 
 const json::Value *ASTNode::findContext() {
-  LLVM_DEBUG(dbgs() << "findContext: AccessorValue[0]=" << AccessorValue[0]
-                    << "\n");
   // The mustache spec allows for dot notation to access nested values
   // a single dot refers to the current context.
   // We attempt to find the JSON context in the current node, if it is not
@@ -792,24 +817,12 @@ const json::Value *ASTNode::findContext() {
   StringRef CurrentAccessor = AccessorValue[0];
   ASTNode *CurrentParent = Parent;
 
-  LLVM_DEBUG(dbgs() << "findContext: ParentContext: ";
-             if (ParentContext) ParentContext->print(dbgs());
-             else dbgs() << "nullptr";
-             dbgs() << "\n");
-
   while (!CurrentContext || !CurrentContext->get(CurrentAccessor)) {
-    LLVM_DEBUG(dbgs() << "findContext: climbing parent\n");
     if (CurrentParent->Ty != Root) {
       CurrentContext = CurrentParent->ParentContext->getAsObject();
       CurrentParent = CurrentParent->Parent;
-      LLVM_DEBUG(dbgs() << "findContext: new ParentContext: ";
-                 if (CurrentParent->ParentContext)
-                   CurrentParent->ParentContext->print(dbgs());
-                 else dbgs() << "nullptr";
-                 dbgs() << "\n");
       continue;
     }
-    LLVM_DEBUG(dbgs() << "findContext: reached root, not found\n");
     return nullptr;
   }
   const json::Value *Context = nullptr;
@@ -825,28 +838,24 @@ const json::Value *ASTNode::findContext() {
       Context = CurrentValue;
     }
   }
-  LLVM_DEBUG(dbgs() << "findContext: found value: ";
-             if (Context) Context->print(dbgs());
-             else dbgs() << "nullptr";
-             dbgs() << "\n");
   return Context;
 }
 
-void ASTNode::renderChild(const json::Value &Contexts, MustacheOutputStream &OS) {
+void ASTNode::renderChild(const json::Value &Contexts,
+                          MustacheOutputStream &OS) {
   for (AstPtr &Child : Children)
     Child->render(Contexts, OS);
 }
 
-void ASTNode::renderPartial(const json::Value &Contexts, MustacheOutputStream &OS,
-                            ASTNode *Partial) {
-  LLVM_DEBUG(dbgs() << "renderPartial (helper): Indentation=" << Indentation
-                    << "\n");
+void ASTNode::renderPartial(const json::Value &Contexts,
+                            MustacheOutputStream &OS, ASTNode *Partial) {
+  LLVM_DEBUG(dbgs() << "[Render Partial Indentation] " << Indentation << "\n");
   AddIndentationStringStream IS(OS, Indentation);
   Partial->render(Contexts, IS);
 }
 
-void ASTNode::renderLambdas(const json::Value &Contexts, MustacheOutputStream &OS,
-                            Lambda &L) {
+void ASTNode::renderLambdas(const json::Value &Contexts,
+                            MustacheOutputStream &OS, Lambda &L) {
   json::Value LambdaResult = L();
   std::string LambdaStr;
   raw_string_ostream Output(LambdaStr);
 | 
| You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp -- llvm/lib/Support/Mustache.cpp
 View the diff from clang-format here.diff --git a/llvm/lib/Support/Mustache.cpp b/llvm/lib/Support/Mustache.cpp
index 178f97030..0098b1d39 100644
--- a/llvm/lib/Support/Mustache.cpp
+++ b/llvm/lib/Support/Mustache.cpp
@@ -864,7 +864,8 @@ void ASTNode::renderChild(const json::Value &Contexts,
 
 void ASTNode::renderPartial(const json::Value &Contexts,
                             MustacheOutputStream &OS, ASTNode *Partial) {
-  LLVM_DEBUG(dbgs() << "[Render Partial Indentation] Indentation: " << Indentation << "\n");
+  LLVM_DEBUG(dbgs() << "[Render Partial Indentation] Indentation: "
+                    << Indentation << "\n");
   AddIndentationStringStream IS(OS, Indentation);
   Partial->render(Contexts, IS);
 }
 | 
a5faef5    to
    e0974e4      
    Compare
  
    ddcaf16    to
    e89315c      
    Compare
  
    e0974e4    to
    d5312d1      
    Compare
  
    e89315c    to
    68c27a3      
    Compare
  
    d5312d1    to
    56b63ac      
    Compare
  
    68c27a3    to
    c2719fb      
    Compare
  
    56b63ac    to
    2819519      
    Compare
  
    f153c6a    to
    f4b4bc8      
    Compare
  
    078165d    to
    a211ef4      
    Compare
  
    f4b4bc8    to
    df32a14      
    Compare
  
    340e2ef    to
    f2fdb9c      
    Compare
  
    df32a14    to
    b22fb3e      
    Compare
  
    bd78555    to
    473adbc      
    Compare
  
    b22fb3e    to
    79eaa0a      
    Compare
  
            
          
                llvm/lib/Support/Mustache.cpp
              
                Outdated
          
        
      |  | ||
| for (char C : Data) { | ||
| LLVM_DEBUG(dbgs() << "IndentationStream: NeedsIndent=" << NeedsIndent | ||
| LLVM_DEBUG(dbgs() << "[IndentationStream] NeedsIndent=" << NeedsIndent | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me where we use FooBar and when we use Foo Bar since there doesn't seem to be much consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one is the odd one out. good catch. let me shore up the rest.
79eaa0a    to
    eb3ba40      
    Compare
  
    | size_t StartPosition = StringRef::npos; | ||
| }; | ||
|  | ||
| static const char *tagKindToString(Tag::Kind K) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ilovepi ,
These two new functions are unused in builds without asserts:
../lib/Support/Mustache.cpp:332:20: error: unused function 'tagKindToString' [-Werror,-Wunused-function]
  332 | static const char *tagKindToString(Tag::Kind K) {
      |                    ^~~~~~~~~~~~~~~
../lib/Support/Mustache.cpp:344:20: error: unused function 'jsonKindToString' [-Werror,-Wunused-function]
  344 | static const char *jsonKindToString(json::Value::Kind K) {
      |                    ^~~~~~~~~~~~~~~~
2 errors generated.
The existing logging was inconsistent, and we logged too many things. This PR introduces a more principled schema, and eliminates many, redundant log lines.

No description provided.