Fix Libdl changes #28953
Fix Libdl changes #28953
Conversation
NEWS.md
Outdated
| ------------------------ | ||
|
|
||
| * The `Libdl` module's methods `dlopen()` and `dlsym()` have been updated | ||
| to return `nothing` as the indicator of a nonexistant symbol or library. |
staticfloat
force-pushed
the
sf/libdl_news
branch
from
ea57fcc
to
9642521
Compare
NEWS.md
Outdated
| ------------------------ | ||
|
|
||
| * The `Libdl` module's methods `dlopen()` and `dlsym()` have been updated | ||
| to return `nothing` as the indicator of a nonexistent symbol or library. |
There was a problem hiding this comment.
This gives the impression that the default behavior changed. Instead, it just gained a keyword.
There was a problem hiding this comment.
The behavior did change; if the library is not found, nothing is returned instead of C_NULL.
There was a problem hiding this comment.
Oh, you're right, I should be talking about dlopen_e() and dlsym_e() here. Those are the ones that changed.
There was a problem hiding this comment.
Oh, actually, I missed that that happened. Those functions are exported and checking their output for NULL is their primary use case. I think we need to change that back and have the _e variants return C_NULL in the failure case.
staticfloat
force-pushed
the
sf/libdl_news
branch
from
9642521
to
33de57c
Compare
|
That completely defeats the purpose of the PR. The entire point of the PR
is to start returning nothing from dlsym_e(), because C_NULL is a valid
response from dlsym.
On Thu, Aug 30, 2018 at 11:02 Keno Fischer ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In NEWS.md
<#28953 (comment)>:
> @@ -11,11 +11,21 @@ Refer to the [Release Notes for
v0.7](https://github.com/JuliaLang/julia/blob/master/HISTORY.md) for a
detailed list of changes from Julia v0.6.
+Standard Library Changes
+------------------------
+
+* The `Libdl` module's methods `dlopen()` and `dlsym()` have been updated
+ to return `nothing` as the indicator of a nonexistent symbol or library.
Oh, actually, I missed that that happened. Those functions are exported
and checking their output for NULL is their primary use case. I think we
need to change that back and have the _e variants return C_NULL in the
failure case.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#28953 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAH_aAfdBu0Hr844vXNGNOy5eAEyaTPSks5uWCiggaJpZM4WSZ_a>
.
--
…-E
|
Well, it's fine to add the behavior for the |
|
Alright. So let’s revert this, and then figure out when the right time to
merge it is. The keyword argument stuff was just an incidental refactor;
the core change is disambiguating a symbol that cannot be found with a
symbol that was found but whose value is NULL.
On Thu, Aug 30, 2018 at 11:32 Keno Fischer ***@***.***> wrote:
That completely defeats the purpose of the PR. The entire point of the PR
is to start returning nothing from dlsym_e(), because C_NULL is a valid
response from dlsym.
Well, it's fine to add the behavior for the dlsym function (no _e) with
the new keyword argument. It's probably not fine to change the behavior of
the existing exported function unless we're doing a major version bump of
the stdlib.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#28953 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAH_aF36nvwdwhjDgjTd-siTuyv-qD0qks5uWC-7gaJpZM4WSZ_a>
.
--
…-E
|
|
The answer is simple. Define |
|
I am guessing this is the source of JuliaGraphics/Cairo.jl#253? |
staticfloat
changed the title
NEWS.md for Libdl.{dlopen,dlsym} changes
Indeed it is. Thanks for spelling it out for me, I didn't think of untying the behavior of |
|
Need to change the tests back probably. LGTM otherwise. |
| ------------------------ | ||
|
|
||
| * The `Libdl` module's methods `dlopen()` and `dlsym()` have gained a | ||
| `throw_error` keyword argument, replacing the now-deprecated `dlopen_e()` |
There was a problem hiding this comment.
Slightly off topic but it seems like this keyword could have been more concisely called error or throw; throw_error feels unpleasantly long. Is it too late to change that?
There was a problem hiding this comment.
No, it's not, but let's get this merged to fix the regression. We can change the keyword name anytime before the release of 1.1.
There was a problem hiding this comment.
Right, I don't mean it should be changed in this PR, just a more general remark.
Forgot to include this in #28888.