Updated PR for the printer design proposal #3259
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
seqan-actions
added
lint
seqan-actions
added
lint
rrahn
changed the title
seqan-actions
added
lint
seqan-actions
added
lint
seqan-actions
added
lint
eseiler
requested changes
Jul 9, 2024
|
Documentation preview available at https://docs.seqan.de/preview/seqan/seqan3/3259 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3259 +/- ##
==========================================
+ Coverage 98.12% 98.14% +0.02%
==========================================
Files 270 271 +1
Lines 11926 11955 +29
Branches 105 104 -1
==========================================
+ Hits 11702 11733 +31
+ Misses 224 222 -2 ☔ View full report in Codecov by Sentry. |
seqan-actions
added
lint
seqan-actions
added
lint
seqan-actions
added
lint
FEATURE: add seqan3::default_printer This PR will resolve seqan/product_backlog#63. This issue is a long-standing open to-do of mine. I hope that you can take it over and push it over the finishing line. The state of the current PR is just a draft of an idea. I'll comment on multiple code locations to point out the advantages of the new design. The major idea of the design is due to the following observation: > We use overload resolution and in particular overload ordering via concepts to determine the order in which we should print Just to give a small example (the more down, the more specialized): ```cpp std::cout //-printable [1] < seqan3::tuple_like // [4] < std::ranges::input_range // [2] < std::vector<uint8_t> // [3] < seqan3::sequence // [3] < char * // [2] std::cout //-printable [1] < char // [5] < seqan3::tuple_like // [4] < seqan3::alphabet // [5] < seqan3::mask // [6] ``` NOTE: that using concepts as overload resolution always uses a partially ordered set, which can be depicted by as a [Hasse Diagram](https://en.wikipedia.org/wiki/Hasse_diagram), and by using the except clauses via `requires` we give it a total order. The idea is simple: * Have a list of printers. * The order of the printers dictates in which order an object should be printed. * We allow that multiple printers might be viable to print a type. * Each `printer<type>` either has a function object / lambda `print` or not; depending on whether the `printer` can print that `type` or not (implemented by [template specialization](https://en.cppreference.com/w/cpp/language/template_specialization)) * We can explicitly use `printer` in other printer's, if we know that only that overload should be used, So put together: For a given type, ask every printer in order whether it can print that type and the first one to answer yes, is the selected printer. ---- [1] If all overloads do not work, use `std::ostream` https://github.com/seqan/seqan3/blob/6b681fb2eae5ab2997d293e99fc6a7f869a20316/include/seqan3/core/debug_stream/debug_stream_type.hpp#L242-L247 [2] Use this for all `std::ranges::input_range`s except if type is something like `std::filesystem` (type == range_value_t) or `char *` https://github.com/seqan/seqan3/blob/6b681fb2eae5ab2997d293e99fc6a7f869a20316/include/seqan3/core/debug_stream/range.hpp#L96-L98 https://github.com/seqan/seqan3/blob/6b681fb2eae5ab2997d293e99fc6a7f869a20316/include/seqan3/core/debug_stream/range.hpp#L38-L45 [3] Same as [2] where value_type is an alphabet but only if the alphabet is not an `unsigned int` (this condition has no test case?!) https://github.com/seqan/seqan3/blob/6b681fb2eae5ab2997d293e99fc6a7f869a20316/include/seqan3/core/debug_stream/range.hpp#L138-L141 [4] Use this for all `std::tuple`-like types except if it is a `std::ranges::input_range` (what is a tuple and ranges at the same time?!) and an `seqan3::alphabet` (basically `seqan3::alphabet_tuple_base` derived types) https://github.com/seqan/seqan3/blob/6b681fb2eae5ab2997d293e99fc6a7f869a20316/include/seqan3/core/debug_stream/tuple.hpp#L53-L56 [5] Use this for all `seqan3::alphabet`s except if it can be printed by `std::cout` (like `char`) https://github.com/seqan/seqan3/blob/6b681fb2eae5ab2997d293e99fc6a7f869a20316/include/seqan3/alphabet/detail/debug_stream_alphabet.hpp#L30-L32 [6] Type must be `seqan3::semialphabet` and `seqan3::mask` https://github.com/seqan/seqan3/blob/6b681fb2eae5ab2997d293e99fc6a7f869a20316/include/seqan3/alphabet/detail/debug_stream_alphabet.hpp#L46-L48 Co-authored-by: seqan-actions[bot] <[email protected]> Co-authored-by: rrahn <[email protected]> Co-authored-by: Enrico Seiler <[email protected]>
eseiler
approved these changes
Aug 2, 2024
There was a problem hiding this comment.
I changed the style:
- Always forward
- Except for PODs
- Do the remove_cvref_t when looking for a suitable printer
Instantiates exactly one printer struct for each distinct type.
Instantiates at most 4 operator()s.
The forwarding reference template argument arg_t is can only be called
through a instance of the printer with the correct type. So arg_t should
usually be the correct type.
I found the old approach harder to understand (more differences amongst
printers). Also the cvref handling by inheritance causes more
structs/operators to be instantiated than needed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR builds upon the original proposal of @marehr (#3227).
I changed from the
printstatic member function to overloading the function call operator of the printer classes.Using this idea, we can now test wether a printer is invocable for a given output stream and the streamable argument.
I also refactored some of the original default_printer code to simplify it.
Among others, the is_printer is not needed anymore as we can use the invocable concept.
For investigating errors I kept the concept
printable_withwhich is basically just an alias for theinvocableconcept.In addition, I added documentation.
Once this is done, we can look for more debug_stream instances that can be replaced with the printer mechanism.