deprecate some macros in tommath.h #223
Conversation
|
@sjaeckel I squashed the two commits to avoid confusion. |
|
@sjaeckel Ready to merge? |
There was a problem hiding this comment.
Yepp, that's a good start, thanks!
PS: if you see another comment from me that doesn't make any sense/is completely outdated or similar: just ignore it, it is most likely just a public sign of me loosing another fight with Github's GUI which is definitely out to kill me! ;-)
|
There is only one issue I discovered - if the deprecated macros are used inside an #ifdef, the pragma blows up with a compile error instead of a warning. An alternative solution for macro deprecation would be the following: #ifndef MP_NO_DEPRECATED_MACROS
# warning deprecated macros enabled, define MP_NO_DEPRECATED_MACROS!
# define DIGIT_BITS MP_DIGIT_BITS
#endif@czurnieden What do you think? |
Looked good to me!? What compiler with what error? |
ew, that sounds nasty how about creating a separate header file there then use this approach
and also protect the header file probably by if it's in a separate file we can simply throw that file away in 2.0.0 after this is resolved the PR LGTM |
|
@czurnieden This is a minor thing and a bit of a trade off what library users will see when they use deprecated macros. Assume a library user does the following: Alternative 1 (in this PR): int a = DIGIT_BITS; /* compiles, but gives deprecation warning */
#if DIGIT_BITS < 32 /* compile error, no deprecation warning! */
Alternative 2 (with MP_NO_DEPRECATED_MACROS): /* ALWAYS prints warning until user defines MP_NO_DEPRECATED_MACROS */
int a = DIGIT_BITS; /* compiles, no warning */
#if DIGIT_BITS < 32 /* compiles, no warning */
|
|
@sjaeckel Ok, I try to fix this. |
* move MP_DEPRECATED to tommath.h since we need it later * add MP_DEPRECATED_PRAGMA
Furthermore we need _MP_NO_DEPRECATED_WARNING to disable the warning on CI. The macro is intentionally prefixed by an underscore to emphasise that its only for internal usage.
|
@sjaeckel See my latest commit introducing the MP_NO_DEPRECATED macro. I didn't add additional headers. Grepping for DEPRECATED will show all the deprecated stuff when we are going to remove it. |
and what do we do with the C standard saying that this means it's UB? How about calling it |
Hmm, I don't think there is a problem. There are other places where I used a _ prefix. But I can change this since INTERNAL_ is a bit more clear. |
It's no real problem AFAICT, just the standard telling us what we're allowed to do and what not... |
| @@ -33,6 +25,10 @@ extern void *MP_CALLOC(size_t nmemb, size_t size); | |||
| extern void MP_FREE(void *mem, size_t size); | |||
| #endif | |||
|
|
|||
| /* TODO: Remove PRIVATE_MP_WARRAY as soon as deprecated MP_WARRAY is removed from tommath.h */ | |||
|
@sjaeckel Hmm, I played a bit with the patched version. I must admit that I don't really like the new solution since it always prints a warning even if nothing deprecated is used. I think this will produce much more annoyance for the users in contrast to Alternative 1, where breakage will only occur if DIGIT_BITS is used in an #ifdef, which is rather improbable. The other possibility would be to just I don't think there is a perfect solution here. But I think Alternative 1 is the most reasonable version, which just breaks early but only in a few rare cases. #226 is the code for Alternative 1 without the MP_NO_DEPRECATED stuff. |
|
closed in favor of #226 |
@sjaeckel Does it make sense to cleanup the public api a bit? This will only give warnings in gcc and probably clang. But I don't see a way to do this more generally.
Edit: I moved the MP_DEPRECATED to the public header since we might need it later. Furthermore I added MP_DEPRECATED_PRAGMA which can be used in macro situations.