jart / jtckdint

C23 Checked Arithmetic
ISC License
87 stars 2 forks source link

> Would love to hear your feedback. #1

Closed Kamilcuk closed 1 year ago

Kamilcuk commented 1 year ago

Weeell, my feedback is that no-gnu path is not worth it. It is a great academic exercise to implement and have fun with, but in real life I use GNU compilers exclusively anyway. I could potentially care about sdcc which is not GNU fully. I do not care about MSVC. So either way, the gnu path is what should be really cared about.

Your code seems to care about MSVC - great! I believe there will come wrappers for MSVC. The comments from gcc source code implementation of _builtin*_overflow helped me a lot in implementation available here: https://github.com/gcc-mirror/gcc/blob/16e2427f50c208dfe07d07f18009969502c25dc8/gcc/internal-fn.c#L724 .

I really like the idea of dynamic dispatch with a switch https://github.com/jart/jtckdint/blob/master/jtckdint.h#L435 . I see superb potential in it, as it will be optimized out anyway with very low optimizations settings. I never liked the _Generic(a, _Generic(b, _Generic(r approach - it takes too long and too much memory to compile, and nested ckd_add(ckd_add(1, 2), 3) generate millions of lines of code from the preprocessor. However, I do not like __ckd_uintmax_t and operations on it. I specifically wanted not to use intmax_t (or int128_t for that matter).

So that's my feedback. Nothing really with substance.

Also you might be interested in my library at https://gitlab.com/Kamcuk/ckd .

jart commented 1 year ago

Thanks!

friendlyanon commented 3 months ago

I made some changes in my fork to make it work with MSVC verifiably. I'm not sure I'm going to upstream it, because the test now uses a 22.4 MB corpus as reference instead of the GCC builtins. I also removed the C++ path, because I only need this for C for now and I wanted to only focus on that codepath.

jart commented 3 months ago

How was the corpus generated?

jart commented 3 months ago

I'm glad that you liked this project enough to make it your own @friendlyanon! The license doesn't require you do this, but I'd encourage you to add yourself to the copyright header and mention the improvements and other changes you made. That way people will know what added value and tradeoffs your fork has to offer compared to the original.

friendlyanon commented 3 months ago

I don't feel that's necessary, becase I only made meaningful changes to the test and I removed the C++ path, as Boost is serving me well there and I only needed this for C.

How was the corpus generated?

By writing the result of the builtins after a successful check to a file when the C11 impl is used https://github.com/friendlyanon/jtckdint/commit/c257a77c08f35cdaed72ed1725c428d52dbbfd0e

jart commented 3 months ago

It's not required. It's just me asking. Whenever I fork/vendor other people's work, I try to document what I've changed. Some licenses like Apache 2.0 legally require doing this. Removing things like all the C++ code diverges from my intent for that file. You're more than welcome to change it however you want, but at that point it shouldn't be written in such a way that it's solely attributed to me.

jart commented 3 months ago

To give you an example, I once took Intel's disassembler Xed and ravaged it down from being a gigantic enterprise codebase to just 700 lines of code. So I added stuff like this to let anyone know it's not the official Xed.

https://github.com/jart/blink/blob/222064a6c325248ccbe5f61f7359dd91ebc29bac/blink/x86.c#L37