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

Can add lexical_cast overloads constrained with enable_if #1021

Merged
merged 1 commit into from
May 2, 2024

Conversation

captainurist
Copy link
Contributor

Also works for overloads constrained with concepts or requirements.

Previously this wasn't working, which is a problem if you want to hook some external serialization framework into CLI11 without painstakingly going through all the types that said external framework supports.

This PR is related to #908.

Copy link

codecov bot commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e4ee3af) to head (f407d2c).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##              main     #1021    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           17        17            
  Lines         4546      4557    +11     
  Branches         0       971   +971     
==========================================
+ Hits          4546      4557    +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@captainurist
Copy link
Contributor Author

captainurist commented Mar 13, 2024

New Codacy issue is a false positive:

class member 'is_lexical_castable::value' is never used.
static constexpr bool value = decltype(test<T, S>(0))::value;

This class member is, in fact, used.

Fuzz / quickfuzz1 segfault is unrelated to my changes.

@phlptp
Copy link
Collaborator

phlptp commented Mar 19, 2024

I think I am ok with this. I want to try it out in a few other contexts just to check a few things.

@captainurist
Copy link
Contributor Author

Any updates here?

@henryiii
Copy link
Collaborator

henryiii commented Apr 7, 2024

I think this seems fine, waiting on @phlptp to check whatever he was going to check.

@captainurist
Copy link
Contributor Author

@phlptp any updates here?

@phlptp
Copy link
Collaborator

phlptp commented Apr 19, 2024

Sorry I am on vacation right now, didn't get a chance to explore before leaving. If you want to merge it I think it is fine, and If I find any issues later I will make some fixes. Otherwise it will be a couple weeks yet.

@captainurist
Copy link
Contributor Author

OK will ping again in May

@phlptp
Copy link
Collaborator

phlptp commented May 2, 2024

Played with this in a few other applications and tests, no issues came up in compilation or testing so I am going to merge it.

@phlptp phlptp merged commit 2e8697c into CLIUtils:main May 2, 2024
54 checks passed
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.

3 participants