aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
In D66850#1649708 <https://reviews.llvm.org/D66850#1649708>, @piscisaureus wrote: > In D66850#1648776 <https://reviews.llvm.org/D66850#1648776>, @aaron.ballman > wrote: > > > LGTM, but missing a test case. > > > > In D66850#1648557 <https://reviews.llvm.org/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. In D66850#1649804 <https://reviews.llvm.org/D66850#1649804>, @piscisaureus wrote: > Add test Thank you! LGTM, do you need someone to commit on your behalf? 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