-
Notifications
You must be signed in to change notification settings - Fork 203
Provide explicit symbols for bn_xxx_l and bn_xxx_ll functions. #321
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
I guess you want these symbols since you want to hide the u32/u64 symbols on msvc6 and instead use the l/ul variants. I disagree with providing different APIs on different compilers. This is not good, things should be consistent across platforms. Note that you can replace the u32/u64 functions with long/long long prototypes on msvc6 due to abi compatibility in the no-stdint-h branch. Only the typedef is not available. I don't buy the argument regarding 128 bit platforms. If there such platforms we can instead add i128/u128 variants. I think we should not merge this PR in its current form. We should first decide what the API should look like and then be consistent. Right now it seems different people are pulling in different directions (stdint vs nostdint for example) and this will lead nowhere and the result won't be good. |
Quite a strong statement and completely wrong. It is only your opinion which you are pushing deeper and deeper into libtommath during last couple of months. Have you got a chance to look into what big guys from top bignum libraries (GMP, OpenSSL/BIGNUM guys, LibreSSL ...) do?
Look, the libtommath is a library which means that the goal is not to have libtommath as nice and shiny DEB/RPM/DLL or whatever else packages. Our goal is (or IMO should be) that libtommath is widely used in final SW products by either linking or bundling or whatever. We are not the most frequently used bignum library; however, our past track record is pretty good and many projects use us (some say it publicly, some not). We also have some "prominent" consumers like tcl, dropbear, rakudo/perl6 that give us feedback or even some coding effort. We cannot ignore them, but we do, we make their life more complicated, what is worse in some case it looks like we complicate their lives for no obvious reasons. By our effort to If we will continue with this approach we will end up in our bubble totally rigorously consistent across platforms but will be losing our users, our consumers. |
I still stand with this statement and would like to hear why it is wrong. I haven't seen an argument from your side beside arguing what other libraries do. The libraries you mentioned and also ltm have a lot of legacy code but the decisions taken back then are not necessarily the right ones forever. If I want to write code which is portable across platforms I need guarantees regarding the size of types. The argument for these imprecise types is usually performance and we still have mp_set which can be used to set a single platform dependent digit. Please read through the discussion in #285 and #278. Both @sjaeckel and @czurnieden voted for using precise types.
Actually we provide mp_get/set_l/ul as macros. Therefore I am not against adding the functions @nijtmans proposed in this PR. However I am not fond of compromising in the light of goals I consider questionable, like avoiding stdint.h.
I agree. But a design discussion should be allowed, which takes the goals into consideration. During such a discussion the maintainers decide which API to provide or not. Furthermore things are not frozen in time and there must be the possibility to improve and remove bad functions. The design discussion should be driven by technical arguments. You refuse to argue about stdint.h. A missing stdint.h can be fixed trivially by providing your own header. This will be a much better solution than doing search and replace on the code base as in your no-stdint-h branch.
Please show me those cases. If they seem questionable I can probably tell you the reason. If I don't manage to do so on technical grounds @sjaeckel can revert the changes. |
I am tired. I am leaving. |
@karel-m , Before you leave, at least put your approval here. From your remarks I conclude you approve ;-) @minad wrote:
Well, Tcl uses "long long" for all calculations, and everything below LLONG_MIN or above LLONG_MAX is handled over to libtommath. So, you are telling me that Tcl is not portable across platforms ??? Then you have been in another school than I have been .... The only function that Tcl uses for setting mp_int's is mp_set_long_long, which is bad because this function expects an unsigned long long: I agree with it's deprecation. But I don't agree with using mp_set_i64 in stead: There is no guarantee that long long is 64 bits, the standard says that long long is at least 64 bits. libtommath still lacks a suitable replacement function in my view. It would mean that I am forced to change Tcl's limit checkout to hardcoded 64-bit values in stead of LLONG_MIN/LLONG_MAX. I'm not prepared to do that, and I'm sure I'm not the only one. Please hear me! I'm not trying to remove or disable or discourage the xxx_i32/64 functions. They simply are not sufficient for everyone. And the following macro's are not what I learned at school. It's as bad as the ##w## constructs used earlier in the macro's (thank God they are gone now). Please consider putting the REAL signatures of functions in tommath.h, and implement them as documented. And provide a long long variant aside the long variant, so Tcl doesn't have to invent its own weel. That's what this PR is about IMHO. /* get integer, set integer (long) */ /* get integer, set integer (unsigned long) */ Enough techical arguments for now. ;-) |
Ok, as I said - I am not 100% opposed to adding those functions. Therefore I added the macros for long since long varies between 32 and 64 bit on common platforms. long long is 64 bit on all platforms I am aware of. Therefore the ll variants are not necessary. Given that I am not happy with having both ll and i64 variants of the functions. They are only needed if you take the standpoint that you want to avoid stdint.h under all circumstances. These days it is good common practice to use stdint. And I don't think we should compromise in order to support no-stdint. We already did in #311 which is ok since macro readability improved, but duplicating apis is not good. Regarding stdint:
I think we should at this point agree on how to proceed. It could well be that we add more apis which rely on stdint.h what shall we do then? See for example #294. Therefore please let's not work against each other and pull in different directions regarding the api. In the long run it is not feasible to have an api which is compatible with stdint and no-stdint. @sjaeckel what do you think? |
Well, in case you ask: I'm not interested in "short"/"int" variants of those functions, "i32", "i64" "long" and "long long" are more than enough for any practical use I can think of. I would approve (u)int128_t and (u)intmax_t variants, but that's stuff for another PR ;-) |
@nijtmans my point here is mostly - how should we proceed in the light of the two incompatible opinions regarding stdint. Therefore I want to have this resolved now. Otherwise the discussion will come up again and again. It seems you want to somehow shape the api such that both an stdint and long variants are available of all functions. I don't find that satisfying. This leads to unnecessary duplication. How should we proceed in such cases in the future? And I still don't understand your opposition in that point. I am all for reducing libc usage to keep this library minimal. But stdint is the most benign header in libc I know. It only contains very well defined things and it is small, can be easily provided by the one porting the library. Unfortunately you refuse to discuss this. |
Hm, I think you are refering to the PR about mp_expt_d and mp_n_root. Well, talking about exponents/roots: Tcl has a hardcoded limit of 28 bits here, because exponents higher than that simply cannot practially be calculated. The reason: it fits in a mp_digit. So, in this case, there is really no need to go beyond 32-bits. I would go for (unsigned) 32-bit there, and - indeed - I would prefer an "unsigned int" parameter, you most likely prefer "uint32_t". But that's a matter of taste: we should choose one or the other, we shouldn't duplicate. Using "long" would be overkill. Does that answer your assumption? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add the missing functions and reorder the prototypes. then I think this is good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest LGTM & sorry for the delay, but I was busy in the last weeks.
…pe(s) last, as in SET macro's
232ee0d
to
58ae5c2
Compare
Please, provide explicit functions for those 8 symbols, in stead of mapping those to the i/u 32/64 versions: There is no guarantee that long or long long are either 32 or 64 bits, there could be platforms where they are 128 bits. Since we have macro's now, it's trivial.
Also I adapted the .tex-file, some symbols were still not documented. Added the long long variants as well.