[ASTDump] Add a flag indicating whether a CXXThisExpr is implicit

[riccibruno]

Summary

There is currently no way to distinguish implicit from explicit CXXThisExpr in the AST dump output.

[steveire]

I have no objection to this, but I wonder whether all state accessible from all nodes should be part of the AST dump. Where do you think the line is? Is there anything else missing currently from other nodes?

[riccibruno]

In D57649#1382453, @steveire wrote:

I have no objection to this, but I wonder whether all state accessible from all nodes should be part of the AST dump. Where do you think the line is? Is there anything else missing currently from other nodes?

I think that this depends on the specific node. It seem that there should be a distinction between state that is "generally useful" (whatever that means) to the average clang developer/user and internal state which is there for a narrow specific purpose. For example (looking at the statement/expression hierarchy):

NullStmt has a flag HasLeadingEmptyMacro => probably not useful to expose.
SwitchStmt has a flag AllEnumCasesCovered which is used for diagnostics => probably not useful to expose.

Additionally some flags are there only to remember which trailing objects are there. I don't think that exposing these is useful (as long as the AST dump output is unambiguous).

Though it is a subjective distinction and in the end if someone need a flag it is going to be added I guess.

[riccibruno]

Thanks for the review !

[aaron.ballman]

In D57649#1382470, @riccibruno wrote:

Though it is a subjective distinction and in the end if someone need a flag it is going to be added I guess.

That's the crux of how we've decided what information to expose in the past. Some folks will expose the information when adding it to an AST node, but not always. More often what happens is that someone finds they need that information to help make an AST dump more readable, then the information gets exposed at that point because there's a use case for it.

[riccibruno]

In D57649#1382477, @aaron.ballman wrote:

In D57649#1382470, @riccibruno wrote:

Though it is a subjective distinction and in the end if someone need a flag it is going to be added I guess.

That's the crux of how we've decided what information to expose in the past. Some folks will expose the information when adding it to an AST node, but not always. More often what happens is that someone finds they need that information to help make an AST dump more readable, then the information gets exposed at that point because there's a use case for it.

Which is exactly why I added this flag. From what I understand the AST dump is mostly a tool used by people hacking on clang, and so this make sense.

lib/AST/TextNodeDumper.cpp

void TextNodeDumper::VisitCXXNamedCastExpr(const CXXNamedCastExpr *Node) {
   OS << ">";
 }
 
 void TextNodeDumper::VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *Node) {
   OS << " " << (Node->getValue() ? "true" : "false");
 }
 
 void TextNodeDumper::VisitCXXThisExpr(const CXXThisExpr *Node) {
+  if (Node->isImplicit())
+    OS << " implicit";
   OS << " this";
 }
 
 void TextNodeDumper::VisitCXXFunctionalCastExpr(
     const CXXFunctionalCastExpr *Node) {
   OS << " functional cast to " << Node->getTypeAsWritten().getAsString() << " <"
      << Node->getCastKindName() << ">";
 }

test/AST/ast-dump-expr.cpp

void f() {
       // CHECK-NEXT: CopyConstructor
       // CHECK-NEXT: MoveConstructor
       // CHECK-NEXT: CopyAssignment
       // CHECK-NEXT: MoveAssignment
       // CHECK-NEXT: Destructor
       // CHECK-NEXT: FieldDecl 0x{{[^ ]*}} <col:8> col:8 implicit 'V *'
       // CHECK-NEXT: CXXMethodDecl
       // CHECK-NEXT: CompoundStmt
-      // CHECK-NEXT: CXXThisExpr 0x{{[^ ]*}} <col:8> 'V *' this
+      // CHECK-NEXT: CXXThisExpr 0x{{[^ ]*}} <col:8> 'V *' implicit this
 
       [*this]{};
       // CHECK: LambdaExpr 0x{{[^ ]*}} <line:[[@LINE-1]]:7, col:15>
       // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}} <col:7> col:7 implicit class definition
       // CHECK-NEXT: DefinitionData lambda
       // CHECK-NEXT: DefaultConstructor
       // CHECK-NEXT: CopyConstructor
       // CHECK-NEXT: MoveConstructor
       // CHECK-NEXT: CopyAssignment
       // CHECK-NEXT: MoveAssignment
       // CHECK-NEXT: Destructor
       // CHECK-NEXT: FieldDecl 0x{{[^ ]*}} <col:8> col:8 implicit 'V'
       // CHECK-NEXT: CXXMethodDecl
       // CHECK-NEXT: CompoundStmt
       // CHECK-NEXT: ParenListExpr 0x{{[^ ]*}} <col:8> 'NULL TYPE'
       // CHECK-NEXT: UnaryOperator 0x{{[^ ]*}} <col:8> '<dependent type>' prefix '*' cannot overflow
-      // CHECK-NEXT: CXXThisExpr 0x{{[^ ]*}} <col:8> 'V *' this
+      // CHECK-NEXT: CXXThisExpr 0x{{[^ ]*}} <col:8> 'V *' implicit this
     }
   };
 
   int b, c;
 
   [](){};
   // CHECK: LambdaExpr 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:8> '(lambda at {{.*}}:[[@LINE-1]]:3)'
   // CHECK-NEXT: CXXRecordDecl 0x{{[^ ]*}} <col:3> col:3 implicit class definition

test/AST/dump.cpp

 };
 
 // CHECK:      |     `-OMPParallelForDirective {{.+}} {{<line:.+:9, col:80>|<col:9, col:80>}}
 // CHECK-NEXT: |       |-OMPDefaultClause {{.+}} <col:26, col:38>
 // CHECK-NEXT: |       |-OMPPrivateClause {{.+}} <col:40, col:49>
 // CHECK-NEXT: |       | `-DeclRefExpr {{.+}} <col:48> 'int' lvalue OMPCapturedExpr {{.+}} 'a' 'int &'
 // CHECK-NEXT: |       |-OMPSharedClause {{.+}} <col:51, col:59>
 // CHECK-NEXT: |       | `-MemberExpr {{.+}} <col:58> 'int' lvalue ->b
-// CHECK-NEXT: |       |   `-CXXThisExpr {{.+}} <col:58> 'S *' this
+// CHECK-NEXT: |       |   `-CXXThisExpr {{.+}} <col:58> 'S *' implicit this
 // CHECK-NEXT: |       |-OMPScheduleClause {{.+}} <col:61, col:79>
 // CHECK-NEXT: |       | `-ImplicitCastExpr {{.+}} <col:78> 'int' <LValueToRValue>
 // CHECK-NEXT: |       |   `-DeclRefExpr {{.+}} <col:78> 'int' lvalue OMPCapturedExpr {{.+}} '.capture_expr.' 'int'
 // CHECK-NEXT: |       `-CapturedStmt {{.+}} <line:[[@LINE-15]]:5, line:[[@LINE-14]]:9>
 // CHECK-NEXT: |         |-CapturedDecl {{.+}} <<invalid sloc>> <invalid sloc> nothrow
 // CHECK-NEXT: |         | |-ForStmt {{.+}} <line:[[@LINE-17]]:5, line:[[@LINE-16]]:9>
 // CHECK:      |         | | `-UnaryOperator {{.+}} <line:[[@LINE-17]]:7, col:9> 'int' lvalue prefix '++'
 // CHECK-NEXT: |         | |   `-DeclRefExpr {{.+}} <col:9> 'int' lvalue OMPCapturedExpr {{.+}} 'a' 'int &'

test/AST/float16.cpp

 //CHECK-NEXT: | | `-CompoundStmt
 //CHECK-NEXT: | |-CXXMethodDecl {{.*}} used func1c '_Float16 (_Float16)
 //CHECK-NEXT: | | |-ParmVarDecl {{.*}} used arg '_Float16'
 //CHECK-NEXT: | | `-CompoundStmt
 //CHECK-NEXT: | |   `-ReturnStmt
 //CHECK-NEXT: | |     `-BinaryOperator {{.*}} '_Float16' '+'
 //CHECK-NEXT: | |       |-ImplicitCastExpr {{.*}} '_Float16' <LValueToRValue>
 //CHECK-NEXT: | |       | `-MemberExpr {{.*}} '_Float16' lvalue ->f1c 0x{{.*}}
-//CHECK-NEXT: | |       |   `-CXXThisExpr {{.*}} 'C1 *' this
+//CHECK-NEXT: | |       |   `-CXXThisExpr {{.*}} 'C1 *' implicit this
 //CHECK-NEXT: | |       `-ImplicitCastExpr {{.*}} '_Float16' <LValueToRValue>
 //CHECK-NEXT: | |         `-DeclRefExpr {{.*}} '_Float16' lvalue ParmVar 0x{{.*}} 'arg' '_Float16'
 //CHECK-NEXT: | |-CXXMethodDecl {{.*}} used func2c '_Float16 (_Float16)' static
 //CHECK-NEXT: | | |-ParmVarDecl {{.*}} used arg '_Float16'
 //CHECK-NEXT: | | `-CompoundStmt
 //CHECK-NEXT: | |   `-ReturnStmt
 //CHECK-NEXT: | |     `-BinaryOperator {{.*}} '_Float16' '*'
 //CHECK-NEXT: | |       |-ImplicitCastExpr {{.*}} '_Float16' <LValueToRValue>