Respect symbol visibility in export calculation#2821
Conversation
65192cd to
1fa76e9
Compare
|
@ryanking13 would you mind looking into the failures here? I don't understand how they are related to the changes and cannot reproduce them locally. |
|
Nevermind, some debugging stuff broke it. |
ryanking13
left a comment
There was a problem hiding this comment.
Thanks, @hoodmane. I have minor code suggestions otherwise LGTM.
Co-authored-by: Gyeongjae Choi <def6488@gmail.com>
e211ab9 to
255ff62
Compare
|
|
||
| result = [] | ||
| insymbol = False | ||
| for line in completedprocess.stdout.split("\n"): |
There was a problem hiding this comment.
Mybe better to move this to a private helper function (that takes in str and returns the result) and additionally give it a few stdout examples in doctests. Would be easier to understand what kind of stdout output we are typically getting and what is done with it.
There was a problem hiding this comment.
This command has a json output format... but frustratingly it outputs not quite valid json. The errors are minor enough that it seemed like it might be possible to fix by find and replace after the fact but it wasn't worth it.
|
Thanks for the reviews @ryanking13 @rth! |
We shouldn't export symbols with
__attribute__((visibility("hidden"))). Numpy has a test to check that symbols with this attribute aren't exported. This fixes that test.