[FIX] don't hard-fail on concept check #1394

h-2 · 2019-12-02T16:59:55Z

You may not do the things you are doing in the signature of the requires expression, because their validity depends on the result of the requires expression.

This leads to hard-failure for types that would normally soft-fail. Since the concept is checked on globally defined operator| this can lead to all sorts of view-resolution failures to land here and hard-fail.

@sarahet Please check if this removes the first error.

codecov · 2019-12-02T18:57:59Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1394   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files         228      228           
  Lines        8731     8731           
=======================================
  Hits         8516     8516           
  Misses        215      215

Impacted Files	Coverage Δ
include/seqan3/argument_parser/argument_parser.hpp	`98.54% <ø> (ø)`	⬆️
include/seqan3/argument_parser/validators.hpp	`88.69% <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 e694d20...cde9729. Read the comment docs.

smehringer · 2019-12-03T12:56:43Z

include/seqan3/argument_parser/validators.hpp

 {
    typename std::remove_reference_t<validator_type>::value_type;

-    { validator(value) } -> void;
+    { validator(typename std::remove_reference_t<validator_type>::value_type{}) } -> void;


I would use decval in case the value type is not default constructible?

Yes, but it doesn't wark because GCC7 and GCC8 at least trigger ODR-use of that statement (although they shouldn't) which results in the compiler telling you that you cannot use declval in an evaluated context. 🤷‍♂️

yeah I've seen this as well, which is annoying.

h-2 · 2019-12-03T16:50:45Z

(do not merge yet, still WIP)

h-2 · 2019-12-03T21:39:07Z

Ok, a different "fix" was necessary, because the problem is that the concept check of GCC doesn't cope with something existing, but being private/protected. It will result in hard errors instead of failing the concept.

But since ::value_type is taken as an indicator for iterators and ranges it's not the best choice anyway. I have just renamed it here which solves all the problems. Is that ok with you, @smehringer ?

h-2 · 2019-12-03T21:39:43Z

include/seqan3/argument_parser/argument_parser.hpp

@@ -293,7 +293,7 @@ class argument_parser
    template <typename option_type, validator validator_type = detail::default_validator<option_type>>
    //!\cond
        requires (argument_parser_compatible_option<option_type> ||
-                  argument_parser_compatible_option<typename option_type::value_type>) &&
+                  argument_parser_compatible_option<std::ranges::range_value_t<option_type>>) &&


this is just a drive-by fix

smehringer · 2019-12-05T08:43:09Z

fine with me. Good thing we are not stable yet...

smehringer · 2019-12-05T08:43:19Z

CI is failing though

rrahn · 2019-12-05T11:04:07Z

CHANGELOG.md

@@ -48,6 +48,8 @@ If possible, provide tooling that performs the changes, e.g. a shell-script.
 * **Changed class signature of input/output file validators:**
  Most user code will be unaffected; to fix possible compiler errors you need to add an empty template parameter list to
  the respective instances (e.g. change `input_file_validator` to `input_file_validator<>`).
+* The member type that denotes which arguments a `validator` can validate has been renamed from `value_type` to


a) why and b) what has this to do with the hard fails on concept checks?

See above

Ok, a different "fix" was necessary, because the problem is that the concept check of GCC doesn't cope with something existing, but being private/protected. It will result in hard errors instead of failing the concept.

(our ranges often have protected or private ::value_type)

marehr · 2019-12-05T11:09:29Z

https://travis-ci.org/seqan/seqan3/jobs/620336210#L5884-L5962

h-2 · 2019-12-05T13:20:17Z

https://travis-ci.org/seqan/seqan3/jobs/620336210#L5884-L5962

I know, I hope to have time for it tomorrow.

rrahn

I would suggest a different name. I also spotted some documentation issues, that could be fixed right along. Many thanks

rrahn · 2019-12-06T08:24:42Z

include/seqan3/argument_parser/validators.hpp

@@ -97,7 +97,7 @@ SEQAN3_CONCEPT validator = std::copyable<remove_cvref_t<validator_type>> &&
 * \ingroup argument_parser
 * \implements seqan3::validator
 *
- * \tparam option_value_type Must be a (container of) arithmetic type(s).
+ * \tparam option_input_type Must be a (container of) arithmetic type(s).


Sorry, but this class has no template parameter

rrahn · 2019-12-06T08:25:22Z

include/seqan3/argument_parser/validators.hpp

@@ -176,12 +176,12 @@ class arithmetic_range_validator
 *
 * \include test/snippet/argument_parser/validators_2.cpp
 */
-template <typename option_value_type>
+template <typename option_input_type>


here tparam is not documented 🤕

rrahn · 2019-12-06T08:28:08Z

CHANGELOG.md

@@ -48,6 +48,8 @@ If possible, provide tooling that performs the changes, e.g. a shell-script.
 * **Changed class signature of input/output file validators:**
  Most user code will be unaffected; to fix possible compiler errors you need to add an empty template parameter list to
  the respective instances (e.g. change `input_file_validator` to `input_file_validator<>`).
+* The member type that denotes which arguments a `validator` can validate has been renamed from `value_type` to
+  `input_type`.


I guess calling it option_value_type would be more fitting. At the end a validator validates the value of an option. Accordingly, we want to know the value type an option represents.

rrahn · 2019-12-06T08:31:06Z

include/seqan3/argument_parser/validators.hpp

@@ -896,14 +896,14 @@ namespace detail
 * The default validator is needed to make the validator parameter of
 * argument_parser::add_option and argument_parser::add_option optional.
 */
-template <typename option_value_type>
+template <typename option_input_type>


tparam not documented

it would be then as well option_value_t

rrahn · 2019-12-06T08:33:12Z

include/seqan3/argument_parser/validators.hpp

@@ -176,12 +176,12 @@ class arithmetic_range_validator
 *
 * \include test/snippet/argument_parser/validators_2.cpp
 */
-template <typename option_value_type>
+template <typename option_input_type>


This would then be option_value_t to not clash with the typedef inside.

rrahn

lgtm

h-2 requested a review from smehringer December 2, 2019 16:59

smehringer requested changes Dec 3, 2019

View reviewed changes

h-2 force-pushed the fix/arg_parser_concept_check branch from 17a41ae to e8f1eca Compare December 3, 2019 21:35

h-2 requested a review from smehringer December 3, 2019 21:39

h-2 commented Dec 3, 2019

View reviewed changes

smehringer approved these changes Dec 5, 2019

View reviewed changes

rrahn reviewed Dec 5, 2019

View reviewed changes

h-2 force-pushed the fix/arg_parser_concept_check branch from e8f1eca to 32f0e0c Compare December 5, 2019 19:44

h-2 requested a review from rrahn December 5, 2019 19:44

rrahn requested changes Dec 6, 2019

View reviewed changes

h-2 force-pushed the fix/arg_parser_concept_check branch from 32f0e0c to 9572910 Compare December 8, 2019 00:25

h-2 requested a review from rrahn December 8, 2019 00:25

rrahn approved these changes Dec 9, 2019

View reviewed changes

[FIX] don't hard-fail on concept check

cde9729

h-2 force-pushed the fix/arg_parser_concept_check branch from 9572910 to cde9729 Compare December 9, 2019 12:57

h-2 merged commit fdbd8ba into seqan:master Dec 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] don't hard-fail on concept check #1394

[FIX] don't hard-fail on concept check #1394

h-2 commented Dec 2, 2019

codecov bot commented Dec 2, 2019 •

edited

smehringer Dec 3, 2019

h-2 Dec 3, 2019

rrahn Dec 6, 2019

h-2 commented Dec 3, 2019

h-2 commented Dec 3, 2019

h-2 Dec 3, 2019

smehringer commented Dec 5, 2019

smehringer commented Dec 5, 2019

rrahn Dec 5, 2019

h-2 Dec 5, 2019

h-2 Dec 5, 2019

marehr commented Dec 5, 2019

h-2 commented Dec 5, 2019

rrahn left a comment

rrahn Dec 6, 2019

rrahn Dec 6, 2019

rrahn Dec 6, 2019

rrahn Dec 6, 2019

rrahn Dec 6, 2019

rrahn Dec 6, 2019

rrahn left a comment

[FIX] don't hard-fail on concept check #1394

[FIX] don't hard-fail on concept check #1394

Conversation

h-2 commented Dec 2, 2019

codecov bot commented Dec 2, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-2 commented Dec 3, 2019

h-2 commented Dec 3, 2019

Choose a reason for hiding this comment

smehringer commented Dec 5, 2019

smehringer commented Dec 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marehr commented Dec 5, 2019

h-2 commented Dec 5, 2019

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 2, 2019 •

edited