Skip to content
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

generic operator int() etc. #133

Merged

Conversation

johnmcfarlane
Copy link
Contributor

I suspect that char is missing from the list of overloads. But more generally, there should be nothing stopping conversion to/from any integer. But for starters, __int128 is missing from the list of overloads and more generally, generic solutions FTW!

@johnmcfarlane johnmcfarlane force-pushed the generic-operator-integral branch from 4a9028e to 6283e80 Compare November 7, 2021 22:47
@ckormanyos
Copy link
Owner

more generally, there should be nothing stopping conversion to/from any integer.

Hi @johnmcfarlane I'm really not sure if there is a standard rule governing this. Consider conversion of uintwide_t to built-in integer. These kinds of conversions are often narrowing casts. My subjective philosophy with UDTs has usually been to make narrowing casts explicit.

This seems like a subjective interpretation on my side, so I'm open either way. I believe Boost.Multiprecision might require explicit narrowing casts. I seem to recall some dialog with motivating reasons for/agains explicit narrowing casts. But I wonder if there is a rule governing this philosophy?

@johnmcfarlane johnmcfarlane force-pushed the generic-operator-integral branch 2 times, most recently from 7b5b90c to ed5b462 Compare November 8, 2021 12:29
@johnmcfarlane
Copy link
Contributor Author

Re. explicit, I assume I'm replacing like for like here. Does the 'explicitness' of these functions change as a result of this PR?

@johnmcfarlane
Copy link
Contributor Author

I should add that my original comment did not refer to explicit, simply that conversation did be possible at all - something the master branch currently does not facilitate.

@johnmcfarlane johnmcfarlane force-pushed the generic-operator-integral branch from ed5b462 to a8ad0ce Compare November 8, 2021 14:04
@johnmcfarlane
Copy link
Contributor Author

But I wonder if there is a rule governing this philosophy?

For completeness, I recommend the C++ Core Guidelines for guidance on matters like this, i.e. style and best practice. E.g. this rule.

@ckormanyos
Copy link
Owner

ckormanyos commented Nov 8, 2021

For completeness, I recommend the C++ Core Guidelines for guidance on matters like this, i.e. style and best practice.

Indeed. Those are good guidelines. It's surprising how often I forget to adhere to some of them, or maybe didn't even have complete awareness of some of the newer ones such as those regarding move semantics correctness. Thanks for sharing.

@johnmcfarlane johnmcfarlane force-pushed the generic-operator-integral branch 3 times, most recently from 167b89e to 301ed38 Compare November 8, 2021 23:09
@ckormanyos
Copy link
Owner

Does the 'explicitness' of these functions change as a result of this PR?

I still need to check this. Sorry about the delay @johnmcfarlane.

@johnmcfarlane johnmcfarlane force-pushed the generic-operator-integral branch from 301ed38 to 9cc8aff Compare November 9, 2021 07:14
@ckormanyos
Copy link
Owner

Does the 'explicitness' of these functions change as a result of this PR?

I still need to check this. Sorry about the delay

Of course we can integrate this @johnmcfarlane. Sorry I missed the fact that the explicit keyword is still present in your template. I think you are aware that GCC's data type __int128 will not test positive for std::is_integral, but I assume you have a workaround for this if you rely on it in your code. I simply neglected with disregard cast to that particular type.

@johnmcfarlane I did, however, add a few more tests to CI, as I had noticed that I had earlier forgotten to run CI on gcc-clang-9. And I also enabled something in the spot tests.

So if you could merge my current master (after #138 gets cycled and merged to master), we will be good to go on merging or not needing any more the CNL branch --- however you see best.

Then i think we will have at least a draft working model for continuing with CNL integration.

@ckormanyos
Copy link
Owner

Hi @johnmcfarlane there are actually 8 new checks in CI's YAML script and also set up in GitHub branch protection. Sorry about the late unannounced change. You probably need to merge wide-integer master here to move forward.

@johnmcfarlane johnmcfarlane force-pushed the generic-operator-integral branch from 9cc8aff to 2934275 Compare November 9, 2021 20:02
@ckormanyos ckormanyos merged commit 2aa5cbe into ckormanyos:master Nov 9, 2021
@ckormanyos
Copy link
Owner

Thank you @johnmcfarlane I kind of lost track with all the activities going on.

Is wide-integer moving in a usable direction now?

@johnmcfarlane
Copy link
Contributor Author

@ckormanyos very much so, yes. I don't know if we were very close earlier in the year or whether you've made changes that really help but almost none of my tests are failing now after I substitute uintwide_t.

One last thing I'm looking at right now is this error. Which related to your earlier comment...

I think you are aware that GCC's data type __int128 will not test positive for std::is_integral, but I assume you have a workaround for this if you rely on it in your code.

It's not as simple as that. It varies depending on toolchain and on the conformance setting. If you build with -std=c++2a (or 17, 14 etc.) then you get strict conformance and __int128 tests negative. But if you build with -std=gnu++2a it passes. A much more thorough exploration can be found in this blog.

The error I linked to was in the only job where I enable strict conformance (via the CMAKE_CXX_EXTENSIONS CMake variable).

A fix probably involves writing two constructors wrapped in #if defined(__SIZEOF_INT128__). LMK if you'd like me to have a go at writing those.

@johnmcfarlane
Copy link
Contributor Author

BTW tests passing isn't necessarily the end of the story. I'm not sure how I want integration to happen. Given the relative scale of the two projects, dropping the one header directly into CNL and maintaining updates by hand might be the easiest thing. Have to think about it. Also there's the previous discussion on things like naming, error handling etc.. Let's revisit that once CI is passing.

@johnmcfarlane johnmcfarlane deleted the generic-operator-integral branch November 10, 2021 00:19
@ckormanyos
Copy link
Owner

ckormanyos commented Nov 10, 2021

@ckormanyos very much so, yes.

That is really good and makes me happy. Thank you.

... don't know if we were very close earlier in the year or whether you've made changes that really help but almost none of my tests are failing now after I substitute uintwide_t

We really got on the right track in spring with your help, @johnmcfarlane. Further improvements throughout summer helped further (some directly motivated through your comments, other clients and my own progression).

  • Interoperate with built-in floating-point types (also mostly constexpr).
  • Increase constexpr correctness even further.
  • Fix several bugs pointed out by clients
  • Clean up the constructor landscape.
  • Activate and handle conversion warnings.
  • Establish C++11,14,17,20 portability on about 20 compilers from 8-bit micros to powerful 64-bit workstations, not only MSVC/GCC/clang, but also microcontroller compilers from 3 commercial suppliers on multiple target architectures.

These led to a much more mature integral type and brought this work pretty close to actually being portable.

@ckormanyos
Copy link
Owner

ckormanyos commented Nov 10, 2021

A fix probably involves writing two constructors wrapped in #if defined(__SIZEOF_INT128__).

I have an existing preprocessor switch called #define WIDE_INTEGER_HAS_LIMB_TYPE_UINT64. This activates the potential instantiation of the 64-bit limb and thereby (indirectly) assumes the existence of __int128. The problem here is that my existing compiler switch is intended to be set manually and the client is expected to know up front if __int128 is there or not.

In the unrelated projects Boost.Math/Multirpecision if believe there is a sequence of compiler queries that attempts to check at compile time for the presence of __int128.

In wide-integer, I do not know if we should expect the client to manually activate the use of __int128 or if we should attempt to detect it via preprocessor queries as you have done. At any rate, i don't want any preprocessor detection to mess up one of my existing preprocessor manually set options so we mith need to mix those logic parts however we end up going.

LMK if you'd like me to have a go at writing those.

Sure @johnmcfarlane. I agree with you and believe that think constructor-from/cast-to signed and unsigned versions of __int128 would go a long way on moving forward and at least reveal any more subtle problems that might lie beneath that first layer of support.

Feel free to make an int28 branch or similar and give it a try. Or would you like me to make a branch?

@ckormanyos
Copy link
Owner

ckormanyos commented Nov 10, 2021

dropping the one header directly into CNL and maintaining updates by hand might be the easiest thing.

Definitely, @johnmcfarlane. johnmcfarlane/cnl will probably be better if it remains as dependency-free as possible --- certainly you'll want to be independent of a dependency on ckormanyos/wide-integer.

If anything big happens on either your side or mine, we can synchronize up any time.

I would really enjoy as a first step, let's see if we can get it in a plug-and-play form for johnmcfarlane/cnl while retaining proper functionality in my repo --- ultimately I want to know that it just fits in cnl. Then at that time of uniform compatibility, break off on self-managed copies with manual sync if needed.

@johnmcfarlane
Copy link
Contributor Author

I'm also really pleased with how it's going. I wouldn't be surprised to find a few more issues because I never got as far as really putting wide types through their paces and will lean more heavily on wide-integer moving forward.

In wide-integer, I do not know if we should expect the client to manually activate the use of __int128 or if we should attempt to detect it via preprocessor queries as you have done.

I support both those things via CNL_USE_INT128. My chosen approach can be viewed here:

  • If CNL_USE_INT128 is not defined, CNL tries to use 128-bit if available.
  • If CNL_USE_INT128 is defined as 0, CNL doesn't use 128-bit.
  • If CNL_USE_INT128 is defined as 1, CNL does use 128-bit. (Hard error if it cannot.)

I should probably make more effort to separate use of 128-bit in wide-integer from 128-bit conversions. And I think wide-integer should probably do the same, hence I recommend wrapping uintwide_t conversions in __SIZEOF_INT128__, and not WIDE_INTEGER_HAS_LIMB_TYPE_UINT64.

Feel free to make an int28 branch or similar and give it a try. Or would you like me to make a branch?

OK. I'll try and get to that this week.

Definitely, @johnmcfarlane. johnmcfarlane/cnl will probably be better if it remains as dependency-free as possible --- certainly you'll want to be independent of a dependency on ckormanyos/wide-integer.

Really, it's just a scale thing. I think a dependency has advantages: fixes from wide-integer can easily be applied to CNL and it is crystal clear to users what the relationship is between the two packages.

Then at that time of uniform compatibility, break off on self-managed copies with manual sync if needed.

I'm assuming manual syncs will be needed from time to time. For this reason, I'd support certain naming and API changes. I don't think they're essential but they have knock-on effects for the quality of CNL.

@ckormanyos
Copy link
Owner

Agreed on all points @johnmcfarlane. Thank you.

Some of the issues mentioned are tracked at #91 and #139.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants