As identified in #829 and #833. Fixes #829.
Since we touch this anyway, this commit additionally makes the identifiers in the benchmark files a little bit more consistent.
This is necessary before we can merge #833. I preferred a separate PR because it makes it easier to see the results of Travis in #833.
As identified in #829 and #833. Fixes #829.
Since we touch this anyway, this commit additionally makes the
identifiers in the benchmark files a little bit more consistent.
FWIW http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2572.pdf
I wonder if we can use clang-tidy or something like that to catch these UBs in the CI
Just realized, are these also UB? :O
Yeah, probably. We use __ types but we don’t declare them, so this fine. But we define types reserved for the standard lib, so I guess this is evil, yes… And this is probably the only case where we may see a real clash in practice in the future, if some compiler defines both int128_t and __int128_t. (Even then is unlikely to cause issues because it’s hopefully the same type.) What we could do is to guard these typedefs with defined(INT128_MAX). Does this make sense? I could add this to the PR.
IMO the entire libary would be more readable if we used typedefs like i32 and u128. But we’d get a huge diff now, and would interrupt all PRs, pretty much like clang-format.
uint128_t into uint128.
I had thought the int128 typedefs only ran if they weren’t already defined. I agree clashing here is not great.
Personally I really don’t like it when applications make local renames of all the generic standard types: Then I have to go digging through the code to make sure their custom type is defined to be the thing I think it is and that it’s consistently the thing I think it is and isn’t something different on some platform or another. Among other issues you end up with interface uglyness where your custom defined types end up in the interfaces and leak into applications. It was excusable back before there were standard fixed-size types, but isn’t really today, and I think today it mostly still lingers on as copying a style that doesn’t make sense anymore.
This is distinct from e.g. making a type for a particular kind of data like a scalar or a field type, because at least that is defined to be fit for a particular purpose. In most cases using a for-application-type is preferable to using generic types (and, in fact, mandated pretty much universally by MISRA C).
If a generic type needs to be renamed for some compatibility reason– e.g. there isn’t a universally available generic type with the required properties, then sure.
I had thought the int128 typedefs only ran if they weren’t already defined.
I added a commit that implements this. I also added MAX and MIN macros
Note: Currently there’s no compiler that provides (u)int128_t because that will open a can of worms due to (u)intmax_t. See http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2425.pdf .
265 # define SECP256K1_WIDEMUL_INT128 1
266 #else
267 # define SECP256K1_WIDEMUL_INT64 1
268 #endif
269 #if defined(SECP256K1_WIDEMUL_INT128)
270+# if !defined(UINT128_MAX) && defined(__SIZEOF_INT128__)
#if is missing a corresponding #endif.
Just to spell it out, the assumption is that a compiler that adds int128_t in the future also defines UINT128_MAX?
Looks good in general. Not sure how useful it is to define constants we don’t use.
Just to spell it out, the assumption is that a compiler that adds int128_t in the future also defines UINT128_MAX?
Yes. And this is implied by the standard as far as I understand it.
Looks good in general. Not sure how useful it is to define constants we don’t use.
Yeah, one of the reasons why I decided for it is that it increases the probability to have a clash be apparent at compile-time (and fail compilation).