Details
- Reviewers
sidorovd aaron.ballman
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
LGTM, but missing a test case.
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.
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.
Thank you!
LGTM, do you need someone to commit on your behalf?
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.
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?
@aaron.ballman
I was able to run the tests, I think this is good to go.
Can you help me get it landed?