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

Fixes disagreement between operator<< available for SFINAE and actually usable #1405

Merged
merged 2 commits into from
Oct 13, 2018
Merged

Conversation

thecppzoo
Copy link
Contributor

Description

Originally I reported a bug, 1403 about how user code provides an insertion overload (an overload for operator<<) for an std type in the global namespace, however, the code fails to compile in Clang at the point Catch tries to use it, at ReusableStringStream::operator<<. This happens because the SFINAE tests for insertion availability happen after using ::operator<<, thus the global overload is available, which activates the Reusable..., but when actually attempted to use them at ReusableStringStream::operator<< they are not available because:

  1. there hasn't been the using ::operator<< at that point
  2. at the namespace Catch there has been one overload for operator<<(std::ostream &, SourceLineInfo const &) that makes the global overloads not available.

This is not a strict bug because the user is providing an overload for types in std in the global namespace, which is not "the same namespace" as indicated in the Catch documentation.

However, adding to the std namespace overloads of operator<< are undefined behavior, and thus the only other choice for a user who wants to allow catch to stream-insert std types would be to put the overloads for std types into Catch, which then are inconvenient to use from the global namespace or the user namespaces. It seems the code tries to use operator<< global overloads, which works in GCC and perhaps other compilers that have broken two-phase lookup, but does not work in Clang which does it correctly in this case.

With the fix presented, users will now be able to provide inserters in either the same namespace as their types (this does not change) or the global namespace, and Catch2 would be able to consistently test for insertion and use the given insertion operations.

The fix is simple, to hoist the using ::operator<< directive to right after introducing an overload into Catch namespace, which is the point at which the global overloads become non-available.

I am thankful to Github user @mpark for his help elucidating ADL.

GitHub Issues

1403
#Closes 1403

@codecov
Copy link

codecov bot commented Oct 10, 2018

Codecov Report

Merging #1405 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1405      +/-   ##
==========================================
- Coverage   80.14%   80.11%   -0.03%     
==========================================
  Files         119      119              
  Lines        3389     3389              
==========================================
- Hits         2716     2715       -1     
- Misses        673      674       +1

@horenmar
Copy link
Member

Nice work on debugging the issue. 👍

I am going to add a test before merging it in though.

@horenmar horenmar merged commit aaaac35 into catchorg:master Oct 13, 2018
thecppzoo added a commit to thecppzoo/Catch2 that referenced this pull request Nov 17, 2018
Since catchorg#1405 was merged and propagated to the single include declaring a user operator<< in the global namespace makes it available to Catch2 string converters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants