piscisaureus added a comment. In D66850#1649865 <https://reviews.llvm.org/D66850#1649865>, @aaron.ballman wrote:
> 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. 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 <https://reviews.llvm.org/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
