3
\$\begingroup\$

Critique?

#include <iostream>

#include "boost/pfr.hpp"

//////////////////////////////////////////////////////////////////////////////
void print_struct(auto&& s) noexcept
{
  using S = std::remove_cvref_t<decltype(s)>;

  [&]<auto ...I>(std::index_sequence<I...>) noexcept
  {
    (
      [&]() noexcept
      {
        if constexpr(I && (I == boost::pfr::tuple_size_v<S> - 1))
        {
          std::cout << ", " << boost::pfr::get<I>(s) << ')';
        }
        else if constexpr(I)
        {
          std::cout << ", " << boost::pfr::get<I>(s);
        }
        else
        {
          std::cout << '(' << boost::pfr::get<I>(s);
        }
      }(),
      ...
    );
  }(std::make_index_sequence<boost::pfr::tuple_size_v<S>>());
}

//////////////////////////////////////////////////////////////////////////////
int main()
{
  struct
  {
    int a;
    float b;
    std::string c;
  } s{1, 2.1, "wow"};

  print_struct(s);
  std::cout << '\n';

  return 0;
}

Here's an example.

\$\endgroup\$
2
  • \$\begingroup\$ Have you looked at boost's example? It just misses surrounding parenthesis... \$\endgroup\$
    – Jarod42
    Aug 23, 2022 at 8:33
  • \$\begingroup\$ @Jarod42 Nope, I meant this post to be a variant of the well-known printing-a-tuple problem. \$\endgroup\$ Aug 23, 2022 at 9:20

2 Answers 2

6
\$\begingroup\$

It only works for some structs

Your print_struct() function only works for types that support aggregate initialization; it's not a perfect generic solution for printing the contents of structs. You might want to add a comment to explain that, and restrict the type of s using a concept so the compiler will be able to print a nice error message.

Simplify

Your code is too complicated; you are handling the start, middle and end of a struct separately, when you only need to handle the first element and the rest separately. It also will not print empty structs correctly. Consider:

void print_struct(auto&& s) noexcept
{
  using S = std::remove_cvref_t<decltype(s)>;
  
  std::cout << '(';

  [&]<auto ...I>(std::index_sequence<I...>) noexcept
  {
    (
      [&]() noexcept
      {
        if constexpr(I)
        {
          std::cout << ", ";
        }

        std::cout << boost::pfr::get<I>(s);
      }(),
      ...
    );
  }(std::make_index_sequence<boost::pfr::tuple_size_v<S>>());

  std::cout << ')';
}

Handle nested structs

If one of the members of s doesn't have an operator<<() overload for std::ostream, but it is a struct, you could call print_struct() recursively.

Handle printing to other streams

Your print_struct() is hardcoded to print to std::cout, but what if you want to print to std::cerr, a file or a std::stringstream? You can easily support this by adding a reference to std::ostream as a parameter to print_struct().

Incorrect use of noexcept

Since you are printing arbitrary types, there is no guarantee whatsoever that your function won't ever throw, so you should remove noexcept.

Ambiguous output

Simply letting the std::ostream formatter format values does not result in an output is guaranteed to be parsable correctly. Consider the following:

struct
{
  int a;
  float b;
  std::string c;
} s{1, 2, "3, 4); DROP TABLE Students;--"};

print_struct(s);

You might want to choose a serialization format that can be parsed properly, like JSON.

\$\endgroup\$
1
\$\begingroup\$

Boost example has already an example for printing aggregate. Format and naming are different, but to adjust to your style, it would be:

template <class T>
void print_struct(const T& x) {
     std::cout << "(";
    const char* sep = "";
    boost::pfr::for_each_field(x, [&](const auto& v) {
        std::cout << std::exchange(sep, ", ");
        std::cout << v;
    });
    std::cout << ")";
}

For your version

void print_struct(auto&& s) noexcept
{
  using S = std::remove_cvref_t<decltype(s)>;
  • operator << might throw, so noexcept is suspicious.
  • You dont actually use forwarding reference, const reference would be enough. and then the using would be unneeded.

->

template <typename S>
void print_struct(const S& s)
{

For

if constexpr(I && (I == boost::pfr::tuple_size_v<S> - 1))
{
  std::cout << ", " << boost::pfr::get<I>(s) << ')';
}
else if constexpr(I)
{
  std::cout << ", " << boost::pfr::get<I>(s);
}
else
{
  std::cout << '(' << boost::pfr::get<I>(s);
}

Your printing is buggy for several cases:

  • empty struct print nothing (probably not expected)
  • struct with one unique element( prints only ", xx)")

parenthesis should be print outside the fold expression separator should be handled only for first or (exclusively) for last argument.

I prefer the std::exchange version of the separator over doing test on index, but I would say it is mostly just style.

Boost already provides way to iterate on each field (but without the index).

\$\endgroup\$
3
  • \$\begingroup\$ I think if constexpr is superior to std::exchange(), but anyway, whatever works. `using ̇ is not needed even in this example. \$\endgroup\$ Aug 23, 2022 at 12:08
  • \$\begingroup\$ @user1095108: I consider both versions as similar and just stylistic difference. (I would bet that performance are similar and time would be taken by std::cout <<) \$\endgroup\$
    – Jarod42
    Aug 23, 2022 at 19:18
  • \$\begingroup\$ Consider that boost::pfr::for_each_field() will do something similar to your fold over an index sequence, the compiler will see that sep is assigned the same pointer in every iteration, so it can remove the redundant stores. \$\endgroup\$
    – G. Sliepen
    Aug 23, 2022 at 19:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.