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

(Unwillingly) protect against overloaded comma operators in decltype #1029

Merged
merged 2 commits into from Feb 6, 2019

Conversation

@morinmorin
Copy link
Contributor

morinmorin commented Feb 3, 2019

It seems that the doc doesn't explicitly ban overloading comma operators. So this PR adds guards against overloaded comma operators by sprinkling void casts.

Here is a (nonsense) test case:

#include <iostream>

#include "fmt/format.h"
#include "fmt/ostream.h"

template <typename OStream>
struct ostream_with_comma_op
{
    std::ostream& os;

    ostream_with_comma_op(std::ostream& _os) : os(_os) {}

    template <typename T>
    ostream_with_comma_op& operator<<(T const& t)
    {
        os << t;
        return *this;
    }
};

template <typename OStream, typename T>
void operator,(ostream_with_comma_op<OStream> const&, T&&);

struct ouch {};

ostream_with_comma_op<std::ostream> operator<<(std::ostream& os, ouch)
{
    os << "ouch";
    return ostream_with_comma_op<std::ostream>(os);
}

int main ()
{
    std::cout << fmt::format("{}", ouch{}) << '\n';
    
    return 0;
}

Another options would be: adding a note to the doc that pathological types are not supported.

@vitaut

This comment has been minimized.

Copy link
Contributor

vitaut commented Feb 4, 2019

Thanks for the PR. Is this more of a theoretical problem or have you encountered a real-world code that hits this? I am leaning towards adding a note to the docs that we don't support this.

@morinmorin

This comment has been minimized.

Copy link
Contributor Author

morinmorin commented Feb 5, 2019

TL;DR - this is only theoretical.

Formatted output is one of headaches in teaching C++. So I sincerely hope that this great library, fmt, will be adopted in C++20! As for teachability, I'm quite happy that fmt also provides a way to support custom types via operator <<, since overloading is easier for novice C++ programmers than template specializations. I was curious about how format uses operator <<, so I checked the code (and learned streambuf). Then I found this

decltype(
  internal::declval<test_stream<Char>&>() << internal::declval<U>(), std::true_type()
)

I don't think this code breaks in real-world use cases at all, but...

  • I thought that it might make sense to have robust codes, since this library is an implementation of the future stdlib component ;)
  • innocent type, say Ostreamable, would break by inserting three lines of codes
struct type_with_comma_op {};
template <typename T> void operator,(type_with_comma_op, T&&);
template <typename T> type_with_comma_op operator<<(T&&, Ostreamable);
  • … << … , … is a real-world code (an Eigen's initialization syntax).

I'm generally in favor of adding constraints that the type should not have overloaded comma operators; but in this case, the (formal) constraint that we need is not so simple because Ostreamable is not the type to forbid overloaded comma operators. So I updated the code rather than the doc.

@vitaut

This comment has been minimized.

Copy link
Contributor

vitaut commented Feb 5, 2019

Fair enough. Could you by any chance add a test at least for the ostream case?

@morinmorin

This comment has been minimized.

Copy link
Contributor Author

morinmorin commented Feb 6, 2019

Thanks, done (but only for the ostream case..., sorry).

@vitaut vitaut merged commit 9a0a24f into fmtlib:master Feb 6, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vitaut

This comment has been minimized.

Copy link
Contributor

vitaut commented Feb 6, 2019

Merged, thanks!

@morinmorin morinmorin deleted the morinmorin:comma_op branch Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.