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

[FIX] fix regex validator for std::filesystem::path #2216

Merged

Conversation

smehringer
Copy link
Member

Resolves #2214

@eseiler
Copy link
Member

eseiler commented Oct 14, 2020

So the problem was that std::filesystem::path models forward_range and that makes it use the wrong overload because it is more constraint than the option_value_type one?

And then weird stuff happened because it validated by iterating over the path and checking each increment?

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #2216 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2216   +/-   ##
=======================================
  Coverage   98.08%   98.08%           
=======================================
  Files         262      262           
  Lines       10679    10680    +1     
=======================================
+ Hits        10474    10475    +1     
  Misses        205      205           
Impacted Files Coverage Δ
include/seqan3/argument_parser/validators.hpp 90.45% <100.00%> (+0.04%) ⬆️

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 8140b10...a78526a. Read the comment docs.

@smehringer
Copy link
Member Author

So the problem was that std::filesystem::path models forward_range and that makes it use the wrong overload because it is more constraint than the option_value_type one?

It's fine that it models forward_range but unfortunately the requires clause is also true, namely that the value_type of the range is convertible to std::string. The range value type of a std::filesystem::path is again a std::filesystem::path 🙄

And then weird stuff happened because it validated by iterating over the path and checking each increment?

Yes, somehow it creates an infinite loop 🤔

@eseiler
Copy link
Member

eseiler commented Oct 14, 2020

Yes, somehow it creates an infinite loop 🤔

Shouldn't the loop happen on compiletime, because that's where the types must be resolved.
I thought it will try to iterate, create some string, then something goes out of scope and then it segfaults...

@smehringer
Copy link
Member Author

Yes, somehow it creates an infinite loop thinking

Shouldn't the loop happen on compiletime, because that's where the types must be resolved.
I thought it will try to iterate, create some string, then something goes out of scope and then it segfaults...

No it compiles fine but the infinite loop is created when iterating over the file path itself. I don't know what the specs say but I think this will just return the path itself for ever and ever

@eseiler
Copy link
Member

eseiler commented Oct 14, 2020

No it compiles fine but the infinite loop is created when iterating over the file path itself. I don't know what the specs say but I think this will just return the path itself for ever and ever

Ah yes, it returns the path itself. I thought that you meant that the function call is recursive before.

@smehringer smehringer force-pushed the argument_parser_fix_regex_validator branch from 89c2c89 to a62cea3 Compare October 14, 2020 16:16
@smehringer
Copy link
Member Author

@marehr actually your proposed changes to the for_each call fixes the error on its own because when it casts the type to string it won't call the function recursively 👍 . I removed the addition to the requires clause again.

Copy link
Member

@marehr marehr left a comment

Choose a reason for hiding this comment

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

thank you 👍

@marehr marehr requested a review from rrahn October 15, 2020 14:34
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

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

Some minor suggestion. Otherwise looks good to me.

include/seqan3/argument_parser/validators.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

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

thanks :shipit:

@smehringer smehringer force-pushed the argument_parser_fix_regex_validator branch from f0c6323 to c5c906c Compare October 16, 2020 09:58
@smehringer smehringer force-pushed the argument_parser_fix_regex_validator branch from c5c906c to a78526a Compare October 16, 2020 10:46
@smehringer smehringer merged commit 8c43f64 into seqan:master Oct 16, 2020
@smehringer smehringer deleted the argument_parser_fix_regex_validator branch November 4, 2020 10:06
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.

Segmentation fault when executing solution6.cpp
4 participants