6

I wanted to write a generic << for any range and I ended up with this:

std::ostream& operator << (std::ostream& out, std::ranges::range auto&& range) {
    using namespace std::ranges;

    if (empty(range)) {
        return out << "[]";
    }

    auto current = begin(range);
    out << '[' << *current;

    while(++current != end(range)) {
        out << ',' << *current;
    }

    return out << ']';
}

Tested like so:

int main() {
    std::vector<int> ints = {1, 2, 3, 4};
    std::cout << ints << '\n';
}

it works perfectly and outputs:

[1,2,3,4]

But, when tested with:

int main() {
    std::vector<int> empty = {};
    std::cout << empty << '\n';
}

it outputs, unexpectedly:

[[,], ]

Running this code with a debugger, I came to a conclusion that the problem with empty range is that we run the return out << "[]";. Some C++ magic decided that my, just written,

std::ostream& operator << (std::ostream& out, std::ranges::range auto&& range);

is a better match then the, provided in <ostream>,

template< class Traits >
basic_ostream<char,Traits>& operator<<( basic_ostream<char,Traits>& os,  
                                        const char* s );

so instead of just sending "[]" to the output stream like we are used to see, it recurses back to itself, but with "[]" as the range argument.

What is the reason for that being a better match? Can I fix this in a more elegant manner compared to sending [ and ] separately?


EDIT: It appears that this is most likely a bug in GCC 10.1.0, since the newer versions reject the code.

Fureeish
  • 12,533
  • 4
  • 32
  • 62
  • 1
    Is your `operator<<` in a namespace or something? I'm getting an [ambiguous](https://godbolt.org/z/x43f16) call. Can you make a [mre]? – cigien Aug 22 '20 at 21:16
  • Keep in mind you're passing a `const char[3]`, not a `const char*` as you say. – chris Aug 22 '20 at 21:21
  • @cigien seems like GCC 10.1.0 likes to accept code that other version do not. It's not inside a namespace and it [compiles on my compiler](https://godbolt.org/z/nbqzav), but it doesn't if I switch it to newer ones. This is the second time today that I see 10.1 behaving differently compared to other versions. – Fureeish Aug 22 '20 at 21:21
  • @chris I am aware, but I simplified the terminology to assume decaying since I didn't see any `const char[N]` overloads for `basic_ostream`. However you are right that `std::ranges::size("[]")` yields `3`. – Fureeish Aug 22 '20 at 21:23
  • My guess is that this is a gcc10.1 bug. 10.2 and trunk give an error. Maybe edit your question to point that out, it's still a valid question anyway. – cigien Aug 22 '20 at 21:25
  • @Fureeish, I was thinking more along the lines of deducing an array and binding a reference to that to make an exact match compared to the decay (a common error for copy constructors vs. "anything" constructors for example). However, I trust the compiler error in the other comment to mean that this doesn't exactly apply here. – chris Aug 22 '20 at 21:27
  • @chris bingo, turning `"[]"` into `static_cast("[]")` fixes the issue. Not sure what should I do with the question though. I am adding an edit hinting to a compiler bug due to inaccuracies between versions, but that does seem like a valid answer, at least to the second part of the question :> – Fureeish Aug 22 '20 at 21:29
  • @Fureeish, Regarding the question, it would be great for an answer to include a link to the bug report or patch that fixes this. The hard part is finding it. – chris Aug 22 '20 at 21:32

1 Answers1

7

I think this shouldn't compile. Let's simplify the example a bit to:

template <typename T> struct basic_thing { };
using concrete_thing = basic_thing<char>;

template <typename T> concept C = true;

void f(concrete_thing, C auto&&); // #1
template <typename T> void f(basic_thing<T>, char const*); // #2

int main() {
    f(concrete_thing{}, "");
}

The basic_thing/concrete_thing mimics what's going on with basic_ostream/ostream. #1 is the overload you're providing, #2 is the one in the standard library.

Clearly both of these overloads are viable for the call we're making. Which one is better?

Well, they're both exact matches in both arguments (yes, char const* is an exact match for "" even though we're undergoing pointer decay, see Why does pointer decay take priority over a deduced template?). So the conversion sequences can't differentiate.

Both of these are function templates, so can't differentiate there.

Neither function template is more specialized than the other - deduction fails in both directions (char const* can't match C auto&& and concrete_thing can't match basic_thing<T>).

The "more constrained" part only applies if the template parameter setup is the same in both cases, which is not true here, so that part is irrelevant.

And... that's it basically, we're out of tiebreakers. The fact that gcc 10.1 accepted this program was a bug, gcc 10.2 no longer does. Although clang does right now, and I believe that's a clang bug. MSVC rejects as ambiguous: Demo.


Either way, there's an easy fix here which is to write [ and then ] as separate characters.

And either way, you probably don't want to write

std::ostream& operator << (std::ostream& out, std::ranges::range auto&& range);

to begin with, since for that to actually work correctly you'd have to stick it in namespace std. Instead, you want to write a wrapper for an arbitrary range and use that instead:

template <input_range V> requires view<V>
struct print_view : view_interface<print_view<V>> {
    print_view() = default;
    print_view(V v) : v(v) { }

    auto begin() const { return std::ranges::begin(v); }
    auto end() const { return std::ranges::end(v); }

    V v;
};

template <range R>
print_view(R&& r) -> print_view<all_t<R>>;

And define your operator<< to print a print_view. That way, this just works and you don't have to deal with these issues. Demo.

Of course, instead of out << *current; you may want to conditionally wrap that in out << print_view{*current}; to be totally correct, but I'll leave that as an exercise.

Barry
  • 286,269
  • 29
  • 621
  • 977
  • The paragraph with the link explained the issue really well, thank you. On a side note - how would sticking the `<<` overload inside `std` help? What do you exactly mean by "*[....] since for that to **actually** work correctly [...]*"? And why would introducing a new view, paired with a different `<<` (still not inside `std`, since that's not allowed) help in the scenarios where my implementation would fail? Could you provide some or link some examples, not to introduce offtopic details? – Fureeish Aug 22 '20 at 21:46
  • @Fureeish Right now that overload is only found if it's visible at the point of use. But if you have any other function that wants to log a type, it probably won't find your `operator<<`, so it won't work. The only way for that to work is ADL, and the only associated namespace in this operator is `std`. Introducing a new view gives you a new associated namespace for that `operator<<` that isn't `std`. – Barry Aug 22 '20 at 21:47
  • Midnight is (again) not the greatest time to explore such details... Sorry, that was super dumb, deleting the comment. Can you elaborate on the `print_view` part though? Is it just the "more correct" way or will it actually help with the ADL stuff? – Fureeish Aug 22 '20 at 21:56
  • 1
    @Fureeish "more correct" isn't me being arbitrary and capricious. It's more correct precisely _because_ it helps with "the ADL stuff". That is, [this works](https://godbolt.org/z/K46j4x). – Barry Aug 22 '20 at 22:00
  • I'd use `out << +"[]";`, because unary `+` is a good way to obfuscate code and guarantee job security. (Please, don't actually write this.) – Casey Mar 24 '21 at 17:56