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

String conversions #421

Merged
merged 2 commits into from
Jan 31, 2020
Merged

String conversions #421

merged 2 commits into from
Jan 31, 2020

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Jan 31, 2020

Fix for Issue #420.

The issue appears to be that we were checking for is_constructible<std::string, T> from a type, but then using a implicit conversion. So that code path was changed to use is_convertible. A separate path was used for cases with !is_convertible and is_constructible

A test of the situation from #420 was created to verify.

@henryiii you might want to check if this resolves #410 as well.

…ions. Discriminate between the is_convertible and is_constructible type traits for object.
@codecov
Copy link

codecov bot commented Jan 31, 2020

Codecov Report

Merging #421 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #421   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines         3580      3582    +2     
=========================================
+ Hits          3580      3582    +2     

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 6b7f6a7...98397da. Read the comment docs.

@henryiii henryiii merged commit ee851c7 into CLIUtils:master Jan 31, 2020
@henryiii henryiii deleted the string_conversions branch January 31, 2020 12:49
@henryiii
Copy link
Collaborator

Thanks! Doesn't seem to solve my problem, though:

https://github.com/GooFit/GooFit/pull/231/checks?check_run_id=419249036

@phlptp
Copy link
Collaborator Author

phlptp commented Jan 31, 2020

The errors looked different enough that I wasn't sure. Tstring is complex enough that it could be a number of different things. What I am thinking about doing is adding another test file that is for testing the compatibility of different types with add_option. App test is starting to get to be a bit to big to manage.

@henryiii
Copy link
Collaborator

I don't think it's just TString (since a small test shows that TString can be added), but it's something that interacts with TString and the subclassing/new namespace.

@henryiii henryiii added this to the v1.9.1 milestone Jun 2, 2020
nksanthosh added a commit to EOSIO/eos that referenced this pull request Aug 3, 2020
This CLI11 update from v1.9.0 to v1.9.1 provides the following improvements: 
- Support relative inclusion [#475](CLIUtils/CLI11#475)
- Fix cases where spaces in paths could break CMake support [#471](CLIUtils/CLI11#471)
- Fix an issue with string conversion [#421](CLIUtils/CLI11#421)
- Cross-compiling improvement for Conan.io [#430](CLIUtils/CLI11#430)
- Fix option group default propagation [#450](CLIUtils/CLI11#450)
- Fix for C++20 [#459](CLIUtils/CLI11#459)
- Support compiling with RTTI off [#461](CLIUtils/CLI11#461)
@nksanthosh nksanthosh mentioned this pull request Aug 3, 2020
7 tasks
dimas1185 pushed a commit to EOSIO/eos that referenced this pull request Aug 4, 2020
This CLI11 update from v1.9.0 to v1.9.1 provides the following improvements: 
- Support relative inclusion [#475](CLIUtils/CLI11#475)
- Fix cases where spaces in paths could break CMake support [#471](CLIUtils/CLI11#471)
- Fix an issue with string conversion [#421](CLIUtils/CLI11#421)
- Cross-compiling improvement for Conan.io [#430](CLIUtils/CLI11#430)
- Fix option group default propagation [#450](CLIUtils/CLI11#450)
- Fix for C++20 [#459](CLIUtils/CLI11#459)
- Support compiling with RTTI off [#461](CLIUtils/CLI11#461)
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.

String-like conversion
2 participants