-
Notifications
You must be signed in to change notification settings - Fork 82
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
FEATURE: add seqan3::default_printer #3227
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3227 +/- ##
==========================================
+ Coverage 98.17% 98.19% +0.01%
==========================================
Files 269 270 +1
Lines 11854 11856 +2
==========================================
+ Hits 11638 11642 +4
+ Misses 216 214 -2 ☔ View full report in Codecov by Sentry. |
template <typename char_t, typename alignment_t> | ||
requires (detail::debug_streamable_tuple<alignment_t> | ||
&& detail::all_model_aligned_seq<detail::tuple_type_list_t<std::remove_cvref_t<alignment_t>>>) | ||
inline debug_stream_type<char_t> & operator<<(debug_stream_type<char_t> & stream, alignment_t && alignment) | ||
{ | ||
#else | ||
template <typename alignment_t> | ||
requires (tuple_like<std::remove_cvref_t<alignment_t>> && detail::all_model_aligned_seq<detail::tuple_type_list_t<std::remove_cvref_t<alignment_t>>>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can see that the actual definition whether a type should be printable by this printer is much easier than before.
@@ -210,13 +210,15 @@ class cigar : public alphabet_tuple_base<cigar, uint32_t, exposition_only::cigar | |||
//!\} | |||
}; | |||
|
|||
#if 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just shows that there are more overloads that I did not touch and it still compiles.
friend debug_stream_type<other_char_t> & operator<<(debug_stream_type<other_char_t> & s, t && v); | ||
#else | ||
friend debug_stream_type<other_char_t> & operator<<(debug_stream_type<other_char_t> & s, t && v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once all seqan3::operator<<
overloads are converted to the printer
design
The following call
seqan3::debug_stream << any_obj << std::endl;
will always only invoke this single seqan3::debug_stream::operator<<
function. Therefore, we have only a single point of entry for our complete printer logic. (should reduce compile time)
That means the stack trace will look something like this:
1. main()
2. seqan3::debug_stream << ...
3. seqan3::default_printer::print(...)
4. seqan3::tuple_printer::print(...)
5. seqan3::debug_stream << ...
6. seqan3::default_printer::print(...)
7. seqan3::sequence_printer::print(...)
This should result in nice and readable stack traces if for some reason a printer somehow fails.
{ | ||
using t_ = std::remove_cvref_t<t>; | ||
|
||
if constexpr(default_printer::is_printable<t_>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is extremely valuable, since we can for the first time check whether a type is printable!!
That also means that for the first time we know that a certain type is NOT printable. So we can give diagnostics at runtime. Thus, no hard compile error just because a print overload was missing. This also means that every type is technically printable now.
This is a very valuable property for debugging. Imagine a major compiler change and something breaks in our print logic. We can comment out the complete code path for every printer and the code would still compile, since we accept EVERY type for printing.
Printing was always a major issue when doing such major compiler upgrades, since we pulled a lot of cross #include
s by the printing logic.
default_printer::print(s, v); | ||
} else { | ||
std::string const msg = std::string{"debug_stream has no print overload for type: "} + typeid(v).name(); | ||
throw std::runtime_error{msg}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, this can also be changed into a compiler error, like this:
throw std::runtime_error{msg}; | |
static_assert(std::is_same<t_, void>, "Type is not printable."); |
Could be even a compile time decision ala SEQAN3_PRINTER_COMPILE_TIME_ERROR
tuple_printer, // concept seqan3::tuple_like<> | ||
std_printer // anything that can be printed by std::ostream | ||
> | ||
{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is literally the order to print everything, top to bottom, from most specific to most general.
Printers that handle explicit types (like std::nullopt
, seqan3::mask
) should be higher up in this order.
concept nonrecursive_range = !std::same_as<std::remove_cvref_t<std::ranges::range_reference_t<rng_t>>, std::remove_cvref_t<rng_t>>; | ||
|
||
template <typename rng_t> | ||
requires std::ranges::input_range<rng_t> && nonrecursive_range<rng_t> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shows that we don't need any "guard" requirement helper, since we can define printer logic locally and don't need to think about the possible global "state" (i.e. other types that might fulfil the same concept but should not be taken).
template <typename char_t, sequence sequence_t> | ||
inline debug_stream_type<char_t> & operator<<(debug_stream_type<char_t> & s, sequence_t && sequence) | ||
requires detail::debug_stream_range_guard<sequence_t> | ||
&& (!detail::is_uint_adaptation_v<std::remove_cvref_t<std::ranges::range_reference_t<sequence_t>>>) | ||
{ | ||
#else | ||
template <typename sequence_t> | ||
requires sequence<sequence_t> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How simple this can be :)
Of course, we still have the cost of ensuring the correct print order, but it can be seen in a single place at the definition of seqan3::default_printer
.
@@ -70,11 +71,24 @@ template <typename char_t, typename tuple_t> | |||
requires (detail::debug_streamable_tuple<tuple_t>) | |||
inline debug_stream_type<char_t> & operator<<(debug_stream_type<char_t> & s, tuple_t && t) | |||
{ | |||
#else | |||
template <typename tuple_type> | |||
requires tuple_like<tuple_type> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
debug_streamable_tuple
= complicated, tuple_like
= straightforward
template <typename> struct std_printer {}; | ||
template <typename> struct char_sequence_printer {}; | ||
template <typename> struct trace_directions_printer {}; | ||
template <typename> struct tuple_printer {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having all forward declarations in one place is nice, since you know what overloads we have. Also, you can search the code base for the real implementation. This was also always complicated to find the correct <<
overload for a given type.
The complete mechanism is based on two-phase_lookup. That means, the include order of the headers can be completely arbitrary. As long as the header is included, which is necessary to print the type, the type will be printed correctly.
Furthermore, we circumvent a lot of ADL lookup time. This could be furthermore improved if we don't use the <<
operator at all in the print definition.
requires (tuple_like<std::remove_cvref_t<alignment_t>> && detail::all_model_aligned_seq<detail::tuple_type_list_t<std::remove_cvref_t<alignment_t>>>) | ||
struct alignment_printer<alignment_t> | ||
{ | ||
constexpr static auto print = [](auto & stream, auto && alignment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this printer definition does not need to know ANYTHING about seqan3::debug_stream
.
Printer Definition is highly local and highly decoupled.
And furthermore, as long as alignment_printer
is not explicitly instantiated, the compiler will just do a quick look if the entity names are defined. That means the risk of weird compiler behaviour, infinite recursion, stupid requires rules, etc.., is mostly circumvented, since the code is "dead"-code for the compiler, as long as it is not USED. This tremendously helps to reduce the risk during major compiler migrations.
Thanks a lot! Can you also add a to-do list with stuff that still must/needs/should be done regarding the code? |
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 old 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 new design ### Before anything, note that the new design does not break any existing code. As existing `seqan3::debugstream << ` overloads, still take place in overload resolution. 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
Not sure if you meant it like this:
Do you see more? |
Resolved by #3259 |
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 old design
The major idea of the design is due to the following observation:
Just to give a small example (the more down, the more specialized):
NOTE: that using concepts as overload resolution always uses a partially ordered set, which can be depicted by as a Hasse Diagram, and by using the except clauses via
requires
we give it a total order.The new design
Before anything, note that the new design does not break any existing code. As existing
seqan3::debugstream <<
overloads, still take place in overload resolution.The idea is simple:
printer<type>
either has a function object / lambdaprint
or not; depending on whether theprinter
can print thattype
or not (implemented by template specialization)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
seqan3/include/seqan3/core/debug_stream/debug_stream_type.hpp
Lines 242 to 247 in 6b681fb
std::ranges::input_range
s except if type is something likestd::filesystem
(type == range_value_t) orchar *
seqan3/include/seqan3/core/debug_stream/range.hpp
Lines 96 to 98 in 6b681fb
seqan3/include/seqan3/core/debug_stream/range.hpp
Lines 38 to 45 in 6b681fb
unsigned int
(this condition has no test case?!)seqan3/include/seqan3/core/debug_stream/range.hpp
Lines 138 to 141 in 6b681fb
std::tuple
-like types except if it is astd::ranges::input_range
(what is a tuple and ranges at the same time?!) and anseqan3::alphabet
(basicallyseqan3::alphabet_tuple_base
derived types)seqan3/include/seqan3/core/debug_stream/tuple.hpp
Lines 53 to 56 in 6b681fb
seqan3::alphabet
s except if it can be printed bystd::cout
(likechar
)seqan3/include/seqan3/alphabet/detail/debug_stream_alphabet.hpp
Lines 30 to 32 in 6b681fb
seqan3::semialphabet
andseqan3::mask
seqan3/include/seqan3/alphabet/detail/debug_stream_alphabet.hpp
Lines 46 to 48 in 6b681fb