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

Enable CTAD for blocked_rangeNd since C++17 #1524

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rarutyun
Copy link

@rarutyun rarutyun commented Oct 7, 2024

Description

Enable CTAD for blocked_rangeNd since C++17 by making the implementation through the inheritance rather than through alias template.

Type of change

  • bug fix - change that fixes an issue
  • new feature - change that adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and some bug fixes
  • not needed

Documentation

  • updated in # - add PR number
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Technically, the change breaks backward compatibility but this is a preview functionality, where breaking change is acceptable

Notify the following users

@pavelkumbrasev, @vossmjp, @akukanov, @isaevil, @aleksei-fedotov, @kboyarinov

Other information

Of course, this PR should take into account the changes made in #1449. I cloned 1449 and checked my modifications there. Didn't find any broken tests (I might miss something, though).

Please provide feedback on my PR if anybody sees any issues with proposed changes

@kboyarinov
Copy link
Contributor

I have tried to test the implementation locally and found that it enables the CTAD only for copy/move constructors of blocked_rangeNd.
I had an idea on how can CTAD for the main constructor can be also enabled. I have opened a separate PR #1525 to add tests and this explicit deduction guide.

@rarutyun
Copy link
Author

rarutyun commented Oct 8, 2024

I have tried to test the implementation locally and found that it enables the CTAD only for copy/move constructors of blocked_rangeNd. I had an idea on how can CTAD for the main constructor can be also enabled. I have opened a separate PR #1525 to add tests and this explicit deduction guide.

@kboyarinov, I know that. The idea of my PR is to enable CTAD in principle, because it doesn't completely work with aliases prior C++20. I didn't write the deduction guides (which is just a "substory" of CTAD) intentionally. It's a different story and I propose to solve it differently. As far as I can tell, your PR doesn't enable everything that necessary (at least in my mind).

@kboyarinov
Copy link
Contributor

I have tried to test the implementation locally and found that it enables the CTAD only for copy/move constructors of blocked_rangeNd. I had an idea on how can CTAD for the main constructor can be also enabled. I have opened a separate PR #1525 to add tests and this explicit deduction guide.

@kboyarinov, I know that. The idea of my PR is to enable CTAD in principle, because it doesn't completely work with aliases prior C++20. I didn't write the deduction guides (which is just a "substory" of CTAD) intentionally. It's a different story and I propose to solve it differently. As far as I can tell, your PR doesn't enable everything that necessary (at least in my mind).

OK, no problem. In that case the patch looks fine for me, except for copyright years that should be actualized.

@rarutyun
Copy link
Author

rarutyun commented Oct 8, 2024

OK, no problem. In that case the patch looks fine for me, except for copyright years that should be actualized.

Thanks. And as you can guess, copyright issue is a 10 seconds fix ;)

@kboyarinov kboyarinov force-pushed the rarutyun/blocked_range_nd_class branch from 3fe65dc to 64610c6 Compare December 20, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants