Conversation
Signed-off-by: Vihan <vihan+github@vihan.org>
|
Hi @vihanb I'll hopefully soon find time to have a look. By the way. Let us try to have smaller PR's over having PR's that add 100% support for a llvm class so that we can land the changes sooner than later. |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thanks for this huge contribution!
Could you please implement unit tests for the added methods the same way as there are already tests for existing code.
For the future. Reviewing is easier if the PR's are smaller. I would appreciate it if you could submit smaller PRs.
| "cmake-js": "^5.2.0", | ||
| "nan": "^2.13.2" | ||
| "nan": "^2.13.2", | ||
| "segfault-handler": "^1.2.0" |
There was a problem hiding this comment.
What's the reason for adding this dependency?
| } | ||
|
|
||
| class DIBasicType extends DIType { | ||
| readonly isSigned: bool; |
There was a problem hiding this comment.
No such method exists on DIBasicType (llvm-node is only a thin wrapper around llvm methods).
I assume it's a simplification for getSignedness. I would prefer exposing the enum and making the return type optional
|
|
||
| NAN_GETTER(DIBasicTypeWrapper::getSignedness) { | ||
| auto* wrapper = DIBasicTypeWrapper::FromValue(info.Holder())->getDIBasicType(); | ||
| bool isSigned = wrapper->getSignedness() == llvm::DIBasicType::Signedness::Signed; |
There was a problem hiding this comment.
What happens if getSignedness returns an absent value?
2 participants
I've been working on adding some DIBuilder bindings this weekend. This is far from finished but I thought I'd open up a PR just to open a discussion about the classes etc.
Signed-off-by: Vihan vihan+github@vihan.org