Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Hi Nandini,
thank you for working on this! If you get memory leaks, I suggest to try to try to reduce the size of the patch. Can you e.g. only translate the InvalidDomainMap? If this works, we can then gradually add more changes?
Mark this as requesting changes to make sure this does not appear as "requiring review" just yet.
Hi Nandini,
thanks for the update. The patch is almost perfect. Just two minor corrections needed. It seems we are lacking some good documentation of get(), copy(), and release(). I will write it down for you in a second. Meanwhile, can you update your patch and see if it is working with the proposed changes.
lib/Analysis/ScopInfo.cpp | ||
---|---|---|
2777 ↗ | (On Diff #106757) | Use copy() here. |
2817 ↗ | (On Diff #106757) | Use copy() here. |
I understood the documentation from https://reviews.llvm.org/D30325#686684. From what I understood, get() is used to obtain a raw pointer that can be passed but that does not have to be consumed.
Right, now the InvalidDomain is indeed consumed. Which is why using get() is not right. Does this make sense? Is there something specific we can or should explain better?
lib/Analysis/ScopInfo.cpp | ||
---|---|---|
2783 ↗ | (On Diff #106757) | InvalidDomain is consumed here. |
2785 ↗ | (On Diff #106757) | InvalidDomain is consumed here. |
Hi Nandini,
this works now. Thanks a lot! I committed the change for you!
There are two good directions to proceed:
- Take other (small) chunks of code and translate them to C++
- Copy/Import and Improve the C++ bindings documentation that you referenced here and add it into our SPINX documentation.
OK, can you share (and maybe document) what you misunderstood? Others likely make similar mistakes, so it is good to have this documented and explained.
Just that, get() is used to obtain a raw pointer that can be passed to but must not be consumed.
OK. Then we should highlight this in *bold* at an obvious place of our documentation.