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

modified option template #285

Merged
merged 5 commits into from
Jul 29, 2019
Merged

modified option template #285

merged 5 commits into from
Jul 29, 2019

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented May 21, 2019

This PR modifies one of the add_option templates allowing two template parameters to allow direct specification of the conversion type if it is not desired to be the same as the assignment type.

For example if you were assigning to variant, you could specify the assignment as a string, int, double, enum, etc.

Also add overloads of lexical cast for types which can take assignment from double or int.

What this would allow is the complete removal of any mention or inclusion of optional from CLI11, and still maintain support for optional types. Optional<string> optional<double> or optional<int> all work with no additional code, and other optional types can be made to work using the two parameter template as long as the underlying type has a lexical cast conversion.

@henryiii Not sure if this is a direction you want to take, but it could completely get rid of a file and add some additional capabilities that we have used in our applications.

If you think it a good direction I will add some more tests to make sure everything works as advertised with the two parameter template.

@phlptp phlptp requested a review from henryiii May 21, 2019 15:20
@phlptp
Copy link
Collaborator Author

phlptp commented May 21, 2019

looks like some issues with the single_file generation due to commenting out some code in Optional.hpp. I will fix it later if this is a useful direction.

@henryiii
Copy link
Collaborator

I think this is a good direction to go. I was beginning to think about ways to remove the explicit inclusion of the optional.

@codecov
Copy link

codecov bot commented May 23, 2019

Codecov Report

Merging #285 into master will decrease coverage by 0.24%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #285      +/-   ##
==========================================
- Coverage     100%   99.75%   -0.25%     
==========================================
  Files          12       12              
  Lines        2880     2892      +12     
==========================================
+ Hits         2880     2885       +5     
- Misses          0        7       +7
Impacted Files Coverage Δ
include/CLI/App.hpp 100% <100%> (ø) ⬆️
include/CLI/TypeTools.hpp 94.16% <75%> (-5.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73aa158...bea4314. Read the comment docs.

@codecov
Copy link

codecov bot commented May 23, 2019

Codecov Report

Merging #285 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #285   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        2887   2928   +41     
=====================================
+ Hits         2887   2928   +41
Impacted Files Coverage Δ
include/CLI/StringTools.hpp 100% <ø> (ø) ⬆️
include/CLI/Validators.hpp 100% <100%> (ø) ⬆️
include/CLI/App.hpp 100% <100%> (ø) ⬆️
include/CLI/TypeTools.hpp 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbd4933...c44e245. Read the comment docs.

@phlptp
Copy link
Collaborator Author

phlptp commented May 26, 2019

I think this is working mostly how I intended now. Got into some strange corners with different compilers.
https://stackoverflow.com/questions/56296149/narrowing-cast-warnings-in-sfinae-evaluation.

As near as i can figure Gcc allows narrowing conversions using {} syntax in some circumstances, other compilers do not. Which isn't necessarily a big problem since it wasn't supposed to in template code, but for certain compilers gcc <5.0 and gcc 7 in C++17 it issued a warning in unevaluated context. I haven't traced this yet but since it was fixed in later compilers I am assuming it was a bug of some sort. Now also ran into some issues with visual studio 2015 and the type_trait is_assignable, so that led to some "interesting" issues with some peculiarly designed classes to them which explicitly deleted all constructors and assignment operators but some particular ones. So in VS2015 it was saying things were assignable but then when you actually tried the assignment it would generate a compile errors. Which kind of made the trait not so useful.

I tried making some different forms of the assignable trait but it must be something with the compiler cause whatever I tried did the same thing. So in the end I just disabled the tests for VS 2015 and earlier. I don't actually think this affects the actual usage much since it is pretty strange classes that trigger it, that were designed to target very specific overloads. In actual code it would be pretty easy to work around.

@phlptp
Copy link
Collaborator Author

phlptp commented May 28, 2019

@henryiii I think this is ready for review.

include/CLI/TypeTools.hpp Show resolved Hide resolved
// Check for streamability
// Based on https://stackoverflow.com/questions/22758291/how-can-i-detect-if-a-type-can-be-streamed-to-an-stdostream

template <typename S, typename T> class is_streamable {
template <typename SS, typename TT>
template <typename T, typename S = std::ostringstream> class is_streamable {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename this to is_ostreamable?

include/CLI/TypeTools.hpp Outdated Show resolved Hide resolved
include/CLI/TypeTools.hpp Outdated Show resolved Hide resolved
include/CLI/TypeTools.hpp Outdated Show resolved Hide resolved
include/CLI/TypeTools.hpp Show resolved Hide resolved
phlptp and others added 2 commits July 26, 2019 09:30
… some notes about it in the README.md

remove the test from visual studio 2015
vs2015 doesn't seem to properly deal with is_assignable in the cases we care about so make a standalone version that is more direct in what we are doing

add version to appveyor and add some notes to the readme

fix a few test cases to make sure code is covered and test a few other paths

remove unneeded enum streaming operator

add some diagnostic escapes around trait code to eliminate gcc Wnarrowing warnings

work specification of the template operations

remove optional add some templates for options conversions

add the two parameter template for add_option
@henryiii
Copy link
Collaborator

Rebased. It looks like the string_view addition in #300 broke this.

@phlptp
Copy link
Collaborator Author

phlptp commented Jul 26, 2019

I guess there are a few oddities about appending string_view to a std::string, It compiles now on visual studio with C++17 and on gcc 9 at least in MSYS now.

@henryiii
Copy link
Collaborator

This almost works if you remove the special string_view patch. Except on GCC 9. Any ideas?

@phlptp
Copy link
Collaborator Author

phlptp commented Jul 27, 2019

I think I fixed it. had to convert lexical cast to take a const std::string & instead so it wasn't converting a temporary variable to a string_view.

Out of curiosity I tried making the value a string_view for add_option. It compiled fine but doesn't work, since the input vector of strings to the callback is constructed on the fly as is deleted soon after, so that will never work.

though I do believe the results function or as will work using string_view since that just does the conversion directly on the results_ variable unless you are using the JOIN as a multioption policy and have more than one input.

@henryiii henryiii merged commit eab92ed into CLIUtils:master Jul 29, 2019
@henryiii henryiii deleted the option_template branch July 29, 2019 04:19
@henryiii henryiii added this to the v1.9 milestone Dec 31, 2019
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