-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Disallow invoking GMP constructor #10158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is a bug please backport to 8.1 |
6ebc9aa
to
6a91fab
Compare
I've update the PR, 8.2 and master additionally need this PR: #10160 |
I'm slightly concerned about no longer allowing |
I don't know, because if anyone is instantiating it via |
When it comes to the creativity of users, almost nothing is unlikely. After all, GMP objects can be constructed normally since they've been invented. Given that there is no need to hurry (next QAs are scheduled for Jan, 19th), it might be a good idea to ask on internals about this change (and maybe on other channels as well). Maybe I'm just too overcautious.
At least the first occurrences are about namespaced GMP classes, so that wouldn't affect this change. Searching for |
sigh at this point we may as well just implement the constructor properly, so one can initialize a GMP object this way. Because I find it just strange to allow instantiating it while not passing any arguments. In any case, this is going to be a nightmare to document. |
Like I said, "maybe I'm just too overcautious". :) |
From the ML discussion, it seems the preferred option is to make a proper constructor that is like |
That makes the most sense to me as well.
Sure, I'll do it soon-ish :) |
Superseded by #10225 |
Implements a proper constructor for GMP as discussed in both GH-10158 and https://externals.io/message/119216. Fixes GH-10155 Closes GH-10225 Signed-off-by: George Peter Banyard <[email protected]>
Fixes GH-10155
Note: I targeted master because I wasn't sure whether this could be considered a BC break.