[PATCH] D67057: [AST][JSON] Generate parent context id from Decl* instead of DeclContext*

[Bert Belder via Phabricator via cfe-commits]

piscisaureus marked an inline comment as done.
piscisaureus added a comment.

Fixed nit, this is ready to land IMO.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67057/new/

https://reviews.llvm.org/D67057



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67057: [AST][JSON] Generate parent context id from Decl* instead of DeclContext*

[Bert Belder via Phabricator via cfe-commits]

piscisaureus updated this revision to Diff 218770.
piscisaureus added a comment.

fix nit


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67057/new/

https://reviews.llvm.org/D67057

Files:
  clang/lib/AST/JSONNodeDumper.cpp
  clang/test/AST/ast-dump-decl-context-json.cpp
  clang/test/AST/ast-dump-decl-json.c
  clang/test/AST/ast-dump-funcs-json.cpp
  clang/test/AST/ast-dump-template-decls-json.cpp

Index: clang/test/AST/ast-dump-template-decls-json.cpp
===
--- clang/test/AST/ast-dump-template-decls-json.cpp
+++ clang/test/AST/ast-dump-template-decls-json.cpp
@@ -2217,7 +2217,7 @@
 // CHECK-NEXT:  "tokLen": 1
 // CHECK-NEXT: }
 // CHECK-NEXT:},
-// CHECK-NEXT:"parentDeclContext": "0x{{.*}}",
+// CHECK-NEXT:"parentDeclContextId": "0x{{.*}}",
 // CHECK-NEXT:"previousDecl": "0x{{.*}}",
 // CHECK-NEXT:"name": "f",
 // CHECK-NEXT:"inner": [
@@ -2264,7 +2264,7 @@
 // CHECK-NEXT:"tokLen": 1
 // CHECK-NEXT:   }
 // CHECK-NEXT:  },
-// CHECK-NEXT:  "parentDeclContext": "0x{{.*}}",
+// CHECK-NEXT:  "parentDeclContextId": "0x{{.*}}",
 // CHECK-NEXT:  "previousDecl": "0x{{.*}}",
 // CHECK-NEXT:  "name": "f",
 // CHECK-NEXT:  "type": {
Index: clang/test/AST/ast-dump-funcs-json.cpp
===
--- clang/test/AST/ast-dump-funcs-json.cpp
+++ clang/test/AST/ast-dump-funcs-json.cpp
@@ -426,7 +426,7 @@
 // CHECK-NEXT:"tokLen": 1
 // CHECK-NEXT:   }
 // CHECK-NEXT:  },
-// CHECK-NEXT:  "parentDeclContext": "0x{{.*}}",
+// CHECK-NEXT:  "parentDeclContextId": "0x{{.*}}",
 // CHECK-NEXT:  "previousDecl": "0x{{.*}}",
 // CHECK-NEXT:  "name": "Test1",
 // CHECK-NEXT:  "type": {
Index: clang/test/AST/ast-dump-decl-json.c
===
--- clang/test/AST/ast-dump-decl-json.c
+++ clang/test/AST/ast-dump-decl-json.c
@@ -226,7 +226,7 @@
 // CHECK-NEXT:  "tokLen": 1
 // CHECK-NEXT: }
 // CHECK-NEXT:},
-// CHECK-NEXT:"parentDeclContext": "0x{{.*}}",
+// CHECK-NEXT:"parentDeclContextId": "0x{{.*}}",
 // CHECK-NEXT:"name": "y",
 // CHECK-NEXT:"tagUsed": "struct",
 // CHECK-NEXT:"completeDefinition": true,
Index: clang/test/AST/ast-dump-decl-context-json.cpp
===
--- /dev/null
+++ clang/test/AST/ast-dump-decl-context-json.cpp
@@ -0,0 +1,286 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ast-dump=json -ast-dump-filter Test %s | FileCheck %s
+
+namespace Test {
+
+namespace NS {
+void Function();
+}
+void NS::Function() {}
+
+struct S {
+  void Method();
+};
+void S::Method() {}
+
+} // namespace Test
+
+// CHECK:  "kind": "NamespaceDecl",
+// CHECK-NEXT:  "loc": {
+// CHECK-NEXT:   "file": "{{.*}}",
+// CHECK-NEXT:   "line": 3,
+// CHECK-NEXT:   "col": 11,
+// CHECK-NEXT:   "tokLen": 4
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "range": {
+// CHECK-NEXT:   "begin": {
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 9
+// CHECK-NEXT:   },
+// CHECK-NEXT:   "end": {
+// CHECK-NEXT:"line": 15,
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 1
+// CHECK-NEXT:   }
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "name": "Test",
+// CHECK-NEXT:  "inner": [
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x[[NS_ID:.*]]",
+// CHECK-NEXT:"kind": "NamespaceDecl",
+// CHECK-NEXT:"loc": {
+// CHECK-NEXT: "line": 5,
+// CHECK-NEXT: "col": 11,
+// CHECK-NEXT: "tokLen": 2
+// CHECK-NEXT:},
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "col": 1,
+// CHECK-NEXT:  "tokLen": 9
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "line": 7,
+// CHECK-NEXT:  "col": 1,
+// CHECK-NEXT:  "tokLen": 1
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"name": "NS",
+// CHECK-NEXT:"inner": [
+// CHECK-NEXT: {
+// CHECK-NEXT:  "id": "0x[[FUNCTION_ID:.*]]",
+// CHECK-NEXT:  "kind": "FunctionDecl",
+// CHECK-NEXT:  "loc": {
+// CHECK-NEXT:   "line": 6,
+// CHECK-NEXT:   "col": 6,
+// CHECK-NEXT:   "tokLen": 8
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "range": {
+// CHECK-NEXT:   "begin": {
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 4
+// CHECK-NEXT:   },
+// CHECK-NEXT:   "end": {
+// CHECK-NEXT:"col": 15,
+// CHECK-NEXT:"tokLen": 1
+// CHECK-NEXT:   }
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "name": "Function",
+// CHECK-NEXT:  "type": {
+// CHECK-NEXT:   "qualType": "void ()"
+// CHECK-NEXT:  }
+// CHECK-NEXT: }
+// CHECK-NEXT:]
+// CHECK-NEXT:   },
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x{{.*}}",
+// CHECK-NEXT:"kind": "FunctionDecl",
+// CHECK-NEXT:"loc": {
+// CHECK-NEXT: "line": 8,
+// CHECK-NEXT: "col": 10,
+// CHECK-NEXT: "tokLen": 8
+// CHECK-NEXT:},
+// 

[PATCH] D67057: [AST][JSON] Generate parent context id from Decl* instead of DeclContext*

[Bert Belder via Phabricator via cfe-commits]

piscisaureus created this revision.
piscisaureus added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Because of multiple inheritance, a DeclContext pointer does not produce
the same pointer representation as a Decl pointer that references the
same AST Node.

When dumping the parentDeclContextId field of a node, convert the pointer
to Decl* first, so the id can be used to find the AST node it references.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67057

Files:
  clang/lib/AST/JSONNodeDumper.cpp
  clang/test/AST/ast-dump-decl-context-json.cpp
  clang/test/AST/ast-dump-decl-json.c
  clang/test/AST/ast-dump-funcs-json.cpp
  clang/test/AST/ast-dump-template-decls-json.cpp

Index: clang/test/AST/ast-dump-template-decls-json.cpp
===
--- clang/test/AST/ast-dump-template-decls-json.cpp
+++ clang/test/AST/ast-dump-template-decls-json.cpp
@@ -2217,7 +2217,7 @@
 // CHECK-NEXT:  "tokLen": 1
 // CHECK-NEXT: }
 // CHECK-NEXT:},
-// CHECK-NEXT:"parentDeclContext": "0x{{.*}}",
+// CHECK-NEXT:"parentDeclContextId": "0x{{.*}}",
 // CHECK-NEXT:"previousDecl": "0x{{.*}}",
 // CHECK-NEXT:"name": "f",
 // CHECK-NEXT:"inner": [
@@ -2264,7 +2264,7 @@
 // CHECK-NEXT:"tokLen": 1
 // CHECK-NEXT:   }
 // CHECK-NEXT:  },
-// CHECK-NEXT:  "parentDeclContext": "0x{{.*}}",
+// CHECK-NEXT:  "parentDeclContextId": "0x{{.*}}",
 // CHECK-NEXT:  "previousDecl": "0x{{.*}}",
 // CHECK-NEXT:  "name": "f",
 // CHECK-NEXT:  "type": {
Index: clang/test/AST/ast-dump-funcs-json.cpp
===
--- clang/test/AST/ast-dump-funcs-json.cpp
+++ clang/test/AST/ast-dump-funcs-json.cpp
@@ -426,7 +426,7 @@
 // CHECK-NEXT:"tokLen": 1
 // CHECK-NEXT:   }
 // CHECK-NEXT:  },
-// CHECK-NEXT:  "parentDeclContext": "0x{{.*}}",
+// CHECK-NEXT:  "parentDeclContextId": "0x{{.*}}",
 // CHECK-NEXT:  "previousDecl": "0x{{.*}}",
 // CHECK-NEXT:  "name": "Test1",
 // CHECK-NEXT:  "type": {
Index: clang/test/AST/ast-dump-decl-json.c
===
--- clang/test/AST/ast-dump-decl-json.c
+++ clang/test/AST/ast-dump-decl-json.c
@@ -226,7 +226,7 @@
 // CHECK-NEXT:  "tokLen": 1
 // CHECK-NEXT: }
 // CHECK-NEXT:},
-// CHECK-NEXT:"parentDeclContext": "0x{{.*}}",
+// CHECK-NEXT:"parentDeclContextId": "0x{{.*}}",
 // CHECK-NEXT:"name": "y",
 // CHECK-NEXT:"tagUsed": "struct",
 // CHECK-NEXT:"completeDefinition": true,
Index: clang/test/AST/ast-dump-decl-context-json.cpp
===
--- /dev/null
+++ clang/test/AST/ast-dump-decl-context-json.cpp
@@ -0,0 +1,286 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ast-dump=json -ast-dump-filter Test %s | FileCheck %s
+
+namespace Test {
+
+namespace NS {
+void Function();
+}
+void NS::Function() {}
+
+struct S {
+  void Method();
+};
+void S::Method() {}
+
+} // namespace Test
+
+// CHECK:  "kind": "NamespaceDecl",
+// CHECK-NEXT:  "loc": {
+// CHECK-NEXT:   "file": "{{.*}}",
+// CHECK-NEXT:   "line": 3,
+// CHECK-NEXT:   "col": 11,
+// CHECK-NEXT:   "tokLen": 4
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "range": {
+// CHECK-NEXT:   "begin": {
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 9
+// CHECK-NEXT:   },
+// CHECK-NEXT:   "end": {
+// CHECK-NEXT:"line": 15,
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 1
+// CHECK-NEXT:   }
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "name": "Test",
+// CHECK-NEXT:  "inner": [
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x[[NS_ID:.*]]",
+// CHECK-NEXT:"kind": "NamespaceDecl",
+// CHECK-NEXT:"loc": {
+// CHECK-NEXT: "line": 5,
+// CHECK-NEXT: "col": 11,
+// CHECK-NEXT: "tokLen": 2
+// CHECK-NEXT:},
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "col": 1,
+// CHECK-NEXT:  "tokLen": 9
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "line": 7,
+// CHECK-NEXT:  "col": 1,
+// CHECK-NEXT:  "tokLen": 1
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"name": "NS",
+// CHECK-NEXT:"inner": [
+// CHECK-NEXT: {
+// CHECK-NEXT:  "id": "0x[[FUNCTION_ID:.*]]",
+// CHECK-NEXT:  "kind": "FunctionDecl",
+// CHECK-NEXT:  "loc": {
+// CHECK-NEXT:   "line": 6,
+// CHECK-NEXT:   "col": 6,
+// CHECK-NEXT:   "tokLen": 8
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "range": {
+// CHECK-NEXT:   "begin": {
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 4
+// CHECK-NEXT:   },
+// CHECK-NEXT:   "end": {
+// CHECK-NEXT:"col": 15,
+// CHECK-NEXT:"tokLen": 1
+// CHECK-NEXT:   }
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "name": "Function",
+// CHECK-NEXT:  "type": {
+// CHECK-NEXT:   "qualType": "void ()"

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Bert Belder via Phabricator via cfe-commits]

piscisaureus added a comment.

@aaron.ballman
I was able to run the tests, I think this is good to go.
Can you help me get it landed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66850/new/

https://reviews.llvm.org/D66850



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Bert Belder via Phabricator via cfe-commits]

piscisaureus updated this revision to Diff 217730.
piscisaureus added a comment.

Add missing CHECK_NEXT line


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66850/new/

https://reviews.llvm.org/D66850

Files:
  clang/lib/AST/JSONNodeDumper.cpp
  clang/test/AST/ast-dump-types-json.cpp
  clang/test/AST/gen_ast_dump_json_test.py

Index: clang/test/AST/gen_ast_dump_json_test.py
===
--- clang/test/AST/gen_ast_dump_json_test.py
+++ clang/test/AST/gen_ast_dump_json_test.py
@@ -20,7 +20,7 @@
 normalize(e)
 elif type(v) is unicode:
 st = v.encode('utf-8')
-if re.match(r"0x[0-9A-Fa-f]+", v):
+if v != "0x0" and re.match(r"0x[0-9A-Fa-f]+", v):
 dict_var[k] = u'0x{{.*}}'
 elif os.path.isfile(v):
 dict_var[k] = u'{{.*}}'
Index: clang/test/AST/ast-dump-types-json.cpp
===
--- clang/test/AST/ast-dump-types-json.cpp
+++ clang/test/AST/ast-dump-types-json.cpp
@@ -20,6 +20,11 @@
 
 typedef int TestQualTypePrinting(const char* c);
 
+typedef int TestUsingShadowDeclType;
+namespace TestNamespaceWithUsingShadowType {
+using ::TestUsingShadowDeclType;
+}
+
 // NOTE: CHECK lines have been autogenerated by gen_ast_dump_json_test.py
 
 
@@ -422,3 +427,75 @@
 // CHECK-NEXT:   }
 // CHECK-NEXT:  ]
 // CHECK-NEXT: }
+
+// CHECK:  "kind": "NamespaceDecl",
+// CHECK-NEXT:  "loc": {
+// CHECK-NEXT:   "file": "{{.*}}",
+// CHECK-NEXT:   "line": 24,
+// CHECK-NEXT:   "col": 11,
+// CHECK-NEXT:   "tokLen": 32
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "range": {
+// CHECK-NEXT:   "begin": {
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 9
+// CHECK-NEXT:   },
+// CHECK-NEXT:   "end": {
+// CHECK-NEXT:"line": 26,
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 1
+// CHECK-NEXT:   }
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "name": "TestNamespaceWithUsingShadowType",
+// CHECK-NEXT:  "inner": [
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x{{.*}}",
+// CHECK-NEXT:"kind": "UsingDecl",
+// CHECK-NEXT:"loc": {
+// CHECK-NEXT: "line": 25,
+// CHECK-NEXT: "col": 9,
+// CHECK-NEXT: "tokLen": 23
+// CHECK-NEXT:},
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "col": 1,
+// CHECK-NEXT:  "tokLen": 5
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"name": "::TestUsingShadowDeclType"
+// CHECK-NEXT:   },
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x{{.*}}",
+// CHECK-NEXT:"kind": "UsingShadowDecl",
+// CHECK-NEXT:"loc": {
+// CHECK-NEXT: "col": 9,
+// CHECK-NEXT: "tokLen": 23
+// CHECK-NEXT:},
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"isImplicit": true,
+// CHECK-NEXT:"target": {
+// CHECK-NEXT: "id": "0x{{.*}}",
+// CHECK-NEXT: "kind": "TypedefDecl",
+// CHECK-NEXT: "name": "TestUsingShadowDeclType"
+// CHECK-NEXT:},
+// CHECK-NEXT:"inner": [
+// CHECK-NEXT: {
+// CHECK-NEXT:  "id": "0x0"
+// CHECK-NEXT: }
+// CHECK-NEXT:]
+// CHECK-NEXT:   }
+// CHECK-NEXT:  ]
+// CHECK-NEXT: }
Index: clang/lib/AST/JSONNodeDumper.cpp
===
--- clang/lib/AST/JSONNodeDumper.cpp
+++ clang/lib/AST/JSONNodeDumper.cpp
@@ -66,6 +66,10 @@
 
 void JSONNodeDumper::Visit(const Type *T) {
   JOS.attribute("id", createPointerRepresentation(T));
+
+  if (!T)
+return;
+
   JOS.attribute("kind", (llvm::Twine(T->getTypeClassName()) + "Type").str());
   JOS.attribute("type", createQualType(QualType(T, 0), /*Desugar*/ false));
   attributeOnlyIfTrue("isDependent", T->isDependentType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Bert Belder via Phabricator via cfe-commits]

piscisaureus added a comment.

In D66850#1649865 , @aaron.ballman 
wrote:

> In D66850#1649708 , @piscisaureus 
> wrote:
>
> > In D66850#1648776 , @aaron.ballman 
> > wrote:
> >
> > > LGTM, but missing a test case.
> > >
> > > In D66850#1648557 , @sidorovd 
> > > wrote:
> > >
> > > > LGTM. I'm not an expert in JSON, but may be it makes sense to move the 
> > > > change a line earlier before creation of pointer representation?
> > >
> > >
> > > I would prefer it remains where it is -- having the `0x0` in the output 
> > > for a null pointer is a good thing because it conveys more information 
> > > than a totally empty `Type` object. We're accidentally inconsistent about 
> > > this currently (Decl prints 0x0 but Stmt gives an empty object).
> >
> >
> > I don't disagree, but I would argue that "0x0" is a rather poor choice, 
> > since now the consumer has to treat the "id" field as an opaque value, 
> > **except** when it's "0x0". A better choice would be to use JavaScript 
> > `null` to represent null pointers.
>
>
> Normally, I'd say yes, but in this case, we have to represent the pointer as 
> a string because the number type in JSON is a signed value. Rather than 
> "you'll either get a string or null", it seemed a bit more friendly to say 
> "you'll always get a string" and allow the consumer to decide how to handle 
> degenerate values. That said, I don't feel super strongly about this.


I agree using numbers isn't feasible. Not because javascript numbers are 
signed, but because they're 64-bit floating point values, which means they 
can't faithfully store 64-bit int values. The latest versions of ECMAScript 
support arbitrary-precision integers a.k.a. BigInt (written as 
`0xABCABCABCABCABCABCn`) which **can** represent 64-bit values, but support 
isn't currently widespread.

The reason I suggest to use `null` (which is not a number nor a string, but a 
type of its own) is that users need to treat the `id` as an opaque value 
(essentially they're only usable to compare nodes and use it as a lookup key in 
a map), with the exception of `"0x0"` which has a specific meaning. As a 
frequent JavaScript/TypeScript user, I would say that using different types is 
actually more ergonomic than using a string and having to know about certain 
strings that indicate a degenerate case.

> 
> 
> In D66850#1649804 , @piscisaureus 
> wrote:
> 
>> Add test
> 
> 
> Thank you!
> 
> LGTM, do you need someone to commit on your behalf?

Yes, please; I have no idea how to commit things to the LLVM tree.
BTW: I haven't ran the full test suite, I suppose some CI system will do that 
before this gets landed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66850/new/

https://reviews.llvm.org/D66850



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Bert Belder via Phabricator via cfe-commits]

piscisaureus updated this revision to Diff 217713.
piscisaureus added a comment.

Add test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66850/new/

https://reviews.llvm.org/D66850

Files:
  clang/lib/AST/JSONNodeDumper.cpp
  clang/test/AST/ast-dump-types-json.cpp
  clang/test/AST/gen_ast_dump_json_test.py

Index: clang/test/AST/gen_ast_dump_json_test.py
===
--- clang/test/AST/gen_ast_dump_json_test.py
+++ clang/test/AST/gen_ast_dump_json_test.py
@@ -20,7 +20,7 @@
 normalize(e)
 elif type(v) is unicode:
 st = v.encode('utf-8')
-if re.match(r"0x[0-9A-Fa-f]+", v):
+if v != "0x0" and re.match(r"0x[0-9A-Fa-f]+", v):
 dict_var[k] = u'0x{{.*}}'
 elif os.path.isfile(v):
 dict_var[k] = u'{{.*}}'
Index: clang/test/AST/ast-dump-types-json.cpp
===
--- clang/test/AST/ast-dump-types-json.cpp
+++ clang/test/AST/ast-dump-types-json.cpp
@@ -20,6 +20,11 @@
 
 typedef int TestQualTypePrinting(const char* c);
 
+typedef int TestUsingShadowDeclType;
+namespace TestNamespaceWithUsingShadowType {
+using ::TestUsingShadowDeclType;
+}
+
 // NOTE: CHECK lines have been autogenerated by gen_ast_dump_json_test.py
 
 
@@ -422,3 +427,74 @@
 // CHECK-NEXT:   }
 // CHECK-NEXT:  ]
 // CHECK-NEXT: }
+
+// CHECK:  "kind": "NamespaceDecl",
+// CHECK-NEXT:  "loc": {
+// CHECK-NEXT:   "line": 24,
+// CHECK-NEXT:   "col": 11,
+// CHECK-NEXT:   "tokLen": 32
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "range": {
+// CHECK-NEXT:   "begin": {
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 9
+// CHECK-NEXT:   },
+// CHECK-NEXT:   "end": {
+// CHECK-NEXT:"line": 26,
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 1
+// CHECK-NEXT:   }
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "name": "TestNamespaceWithUsingShadowType",
+// CHECK-NEXT:  "inner": [
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x{{.*}}",
+// CHECK-NEXT:"kind": "UsingDecl",
+// CHECK-NEXT:"loc": {
+// CHECK-NEXT: "line": 25,
+// CHECK-NEXT: "col": 9,
+// CHECK-NEXT: "tokLen": 23
+// CHECK-NEXT:},
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "col": 1,
+// CHECK-NEXT:  "tokLen": 5
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"name": "::TestUsingShadowDeclType"
+// CHECK-NEXT:   },
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x{{.*}}",
+// CHECK-NEXT:"kind": "UsingShadowDecl",
+// CHECK-NEXT:"loc": {
+// CHECK-NEXT: "col": 9,
+// CHECK-NEXT: "tokLen": 23
+// CHECK-NEXT:},
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"isImplicit": true,
+// CHECK-NEXT:"target": {
+// CHECK-NEXT: "id": "0x{{.*}}",
+// CHECK-NEXT: "kind": "TypedefDecl",
+// CHECK-NEXT: "name": "TestUsingShadowDeclType"
+// CHECK-NEXT:},
+// CHECK-NEXT:"inner": [
+// CHECK-NEXT: {
+// CHECK-NEXT:  "id": "0x0"
+// CHECK-NEXT: }
+// CHECK-NEXT:]
+// CHECK-NEXT:   }
+// CHECK-NEXT:  ]
+// CHECK-NEXT: }
Index: clang/lib/AST/JSONNodeDumper.cpp
===
--- clang/lib/AST/JSONNodeDumper.cpp
+++ clang/lib/AST/JSONNodeDumper.cpp
@@ -66,6 +66,10 @@
 
 void JSONNodeDumper::Visit(const Type *T) {
   JOS.attribute("id", createPointerRepresentation(T));
+
+  if (!T)
+return;
+
   JOS.attribute("kind", (llvm::Twine(T->getTypeClassName()) + "Type").str());
   JOS.attribute("type", createQualType(QualType(T, 0), /*Desugar*/ false));
   attributeOnlyIfTrue("isDependent", T->isDependentType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Bert Belder via Phabricator via cfe-commits]

piscisaureus added a comment.

In D66850#1648776 , @aaron.ballman 
wrote:

> LGTM, but missing a test case.
>
> In D66850#1648557 , @sidorovd wrote:
>
> > LGTM. I'm not an expert in JSON, but may be it makes sense to move the 
> > change a line earlier before creation of pointer representation?
>
>
> I would prefer it remains where it is -- having the `0x0` in the output for a 
> null pointer is a good thing because it conveys more information than a 
> totally empty `Type` object. We're accidentally inconsistent about this 
> currently (Decl prints 0x0 but Stmt gives an empty object).


I don't disagree, but I would argue that "0x0" is a rather poor choice, since 
now the consumer has to treat the "id" field as an opaque value, **except** 
when it's "0x0". A better choice would be to use JavaScript `null` to represent 
null pointers.
That said, I've simply followed the pattern established in `visit(Decl*)` here; 
switching to `null` would warrant a separate patch IMO.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66850/new/

https://reviews.llvm.org/D66850



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66850: Avoid crash when dumping NULL Type as JSON

[Bert Belder via Phabricator via cfe-commits]

piscisaureus updated this revision to Diff 217545.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66850/new/

https://reviews.llvm.org/D66850

Files:
  clang/lib/AST/JSONNodeDumper.cpp


Index: clang/lib/AST/JSONNodeDumper.cpp
===
--- clang/lib/AST/JSONNodeDumper.cpp
+++ clang/lib/AST/JSONNodeDumper.cpp
@@ -66,6 +66,10 @@
 
 void JSONNodeDumper::Visit(const Type *T) {
   JOS.attribute("id", createPointerRepresentation(T));
+
+  if (!T)
+return;
+
   JOS.attribute("kind", (llvm::Twine(T->getTypeClassName()) + "Type").str());
   JOS.attribute("type", createQualType(QualType(T, 0), /*Desugar*/ false));
   attributeOnlyIfTrue("isDependent", T->isDependentType());


Index: clang/lib/AST/JSONNodeDumper.cpp
===
--- clang/lib/AST/JSONNodeDumper.cpp
+++ clang/lib/AST/JSONNodeDumper.cpp
@@ -66,6 +66,10 @@
 
 void JSONNodeDumper::Visit(const Type *T) {
   JOS.attribute("id", createPointerRepresentation(T));
+
+  if (!T)
+return;
+
   JOS.attribute("kind", (llvm::Twine(T->getTypeClassName()) + "Type").str());
   JOS.attribute("type", createQualType(QualType(T, 0), /*Desugar*/ false));
   attributeOnlyIfTrue("isDependent", T->isDependentType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66850: Do not attempt visit child nodes of NULL type

[Bert Belder via Phabricator via cfe-commits]

piscisaureus created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66850

Files:
  clang/include/clang/AST/ASTNodeTraverser.h


Index: clang/include/clang/AST/ASTNodeTraverser.h
===
--- clang/include/clang/AST/ASTNodeTraverser.h
+++ clang/include/clang/AST/ASTNodeTraverser.h
@@ -130,9 +130,9 @@
 
   void Visit(const Type *T) {
 getNodeDelegate().AddChild([=] {
-  getNodeDelegate().Visit(T);
   if (!T)
 return;
+  getNodeDelegate().Visit(T);
   TypeVisitor::Visit(T);
 
   QualType SingleStepDesugar =


Index: clang/include/clang/AST/ASTNodeTraverser.h
===
--- clang/include/clang/AST/ASTNodeTraverser.h
+++ clang/include/clang/AST/ASTNodeTraverser.h
@@ -130,9 +130,9 @@
 
   void Visit(const Type *T) {
 getNodeDelegate().AddChild([=] {
-  getNodeDelegate().Visit(T);
   if (!T)
 return;
+  getNodeDelegate().Visit(T);
   TypeVisitor::Visit(T);
 
   QualType SingleStepDesugar =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits