rollbear / trompeloeil

Header only C++14 mocking framework
Boost Software License 1.0
812 stars 86 forks source link

[fixed on master] `compiling_tests` fails with `clang++-6.0 -std=c++17` #94

Closed AndrewPaxie closed 6 years ago

AndrewPaxie commented 6 years ago

Summary

clang++-6.0 on Ubuntu generates an error when compiling Trompeloeil in std=c++17 or std=c++2a modes.

The latest version of Clang also generates this error.

Environment

Operating system: Ubuntu 18.04 LTS (Bionic Beaver)

Compiler: clang++-6.0 and clang++ pre 7.0.0 a.k.a. clang++-latest.

clang++-6.0 is from package clang-6.0 1:6.0-1ubuntu2.

clang++-latest is locally built each day. This build current as at 26 June 2018.

Flags: -std=c++17 or -std=c++2a.

Command line

clang++-6.0 \
-std=c++17 \
-Weverything \
-Wno-c++98-compat-pedantic \
-Wno-padded \
-Wno-weak-vtables \
-Wno-exit-time-destructors \
-Wno-global-constructors \
-I build/catch \
-I ./include \
-o compiling_tests \
test/compiling_tests.cpp \
test/compiling_tests_11.cpp \
test/compiling_tests_14.cpp

Expected behaviour

Expected compiling_tests to build without error.

Actual behaviour

Compile error

/usr/bin/../lib/gcc/x86_64-linux-gnu/8.0.1/../../../../include/c++/8.0.1/type_traits:945:14: error: base class has incomplete type
    : public is_constructible<_Tp, const _Tp&>
      ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

// ::: (some backtrace information omitted)

test/compiling_tests.hpp:257:3: note: in instantiation of function template specialization 'trompeloeil::call_matcher<void (int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > &),
      std::tuple<trompeloeil::wildcard, trompeloeil::wildcard> >::call_matcher<const trompeloeil::wildcard &, const trompeloeil::wildcard &>' requested here
  MAKE_MOCK2(func, void(int, std::string&), override);
  ^

// ::: (some more backtrace information omitted)

test/compiling_tests_11.cpp:178:26: note: in instantiation of function template specialization 'mock_c::trompeloeil_l_tag_type_trompeloeil_257::func<const trompeloeil::wildcard &, const trompeloeil::wildcard &>' requested here
    REQUIRE_CALL_V(obj2, func(_, _),
                         ^

Further information

This error was encountered as I was gathering information to enable builds of Trompeloeil for C++17 and C++2a.

These build configurations succeed (this is a subset of all the builds that succeed, with a focus on c++17 and c++2a):

                Language    Standard
Compiler        Standard    Library     Comment
----------      --------    --------    -------
g++-5           c++17       stdc++      OK - no warnings, Pass
g++-5           c++17       c++         OK - no warnings, Pass

g++-6           c++17       stdc++      OK - no warnings, Pass
g++-6           c++17       c++         OK - no warnings, Pass

g++-7           c++17       stdc++      OK - no warnings, Pass
g++-7           c++17       c++         OK - no warnings, Pass

g++-8           c++17       stdc++      OK - no warnings, Pass
g++-8           c++17       c++         OK - no warnings, Pass
g++-8           c++2a       stdc++      OK - no warnings, Pass
g++-8           c++2a       c++         OK - no warnings, Pass

g++-latest      c++17       stdc++      OK - no warnings, Pass
g++-latest      c++17       c++         OK - no warnings, Pass
g++-latest      c++2a       stdc++      OK - no warnings, Pass
g++-latest      c++2a       c++         OK - no warnings, Pass

clang++-5.0     c++17       stdc++ v8   OK - no warnings, Pass
clang++-5.0     c++17       c++         OK - no warnings, Pass
clang++-5.0     c++2a       stdc++ v8   OK - no warnings, Pass
clang++-5.0     c++2a       c++         OK - no warnings, Pass

clang++-6.0     c++11       stdc++ v8   OK - no warnings, Pass
clang++-6.0     c++11       c++         OK - no warnings, Pass
clang++-6.0     c++14       stdc++ v8   OK - no warnings, Pass
clang++-6.0     c++14       c++         OK - no warnings, Pass

clang++-latest  c++11       stdc++ v8   OK - no warnings, Pass
clang++-latest  c++11       c++         OK - no warnings, Pass
clang++-latest  c++14       stdc++ v8   OK - no warnings, Pass
clang++-latest  c++14       c++         OK - no warnings, Pass

Note that clang++-6.0 and clang++-latest succeeded in c++11 and c++14 modes.

Either something is wrong with clang++-6.0 in the c++17 and c++2a modes, or Clang has implemented a change from a proposal that has has had an affect on type deduction.

A quick look at the Clang bugzilla does not immediately reveal a previously filed defect report describing this problem.

It is the return of the user-defined conversion problems with wildcard, but I'm left a little concerned there is a limited chance of a workaround in the library code.

I'm raising this issue here, now, before I reduce the test case to the minimum required to reproduce the issue. I will add a comment with the test case once I have it.

Ideas or pointers on how to resolve this are welcomed.

AndrewPaxie commented 6 years ago

This is the smallest example involving Trompeloeil I could find that generates the error,

#include <trompeloeil.hpp>

#include <string>

using trompeloeil::_;

class mock_c
{
public:
  MAKE_MOCK2(func, void(int, std::string&));
};

int
main()
{
  mock_c obj;

  REQUIRE_CALL(obj, func(_, _));

  return 0;
}

This can be run in Compiler Explorer (with trompeloeil.hpp inlined), compiler x86-64 clang++-6.0.0, command line -std=c++17, and the error is reproduced.

Now to remove Trompeloeil from the test case.

rollbear commented 6 years ago

I'll have a look too.

BTW, for experiments in compiler explorer, did you know you can #include URLs?

// way cool URL include!
#include <https://raw.githubusercontent.com/rollbear/trompeloeil/master/include/trompeloeil.hpp>

#include <string>

using trompeloeil::_;

class mock_c
{
public:
  MAKE_MOCK2(func, void(int, std::string&));
};

int
main()
{
  mock_c obj;

  REQUIRE_CALL(obj, func(_, _));

  return 0;
}
rollbear commented 6 years ago

I also notice that clang++ has this problem both with -stdlib=libc++ and with -stdlib=libstdc++.

rollbear commented 6 years ago

I must correct myself here, as I confused two issues.

Your minimal program does compile with clang++-6, with -std=c++17, if -stdlib=libc++ is used, but not if -stdlib=libstdc++.

However, the minimal program is too nice to clang++ compared to compiling_tests, because the lattecr ICEs clang++, regardless of which stdlib to use. Maybe the two are related, but it's not obvious.

rollbear commented 6 years ago

As I suspected, they're unrelated. Here's a "fix" for the clang++ compilation error with -stdlib=libstdc++ when using C++17.

diff --git a/include/trompeloeil.hpp b/include/trompeloeil.hpp
index 57d1ea9..bb02fc0 100644
--- a/include/trompeloeil.hpp
+++ b/include/trompeloeil.hpp
@@ -1648,6 +1648,7 @@ namespace trompeloeil
     // g++ 4.8 gives a "conversion from <T> to <U> is ambiguous" error
     // if this operator is defined.
     template <typename T,
+             typename = detail::enable_if_t<!std::is_same<std::decay_t<T>, wildcard>::value>,
               typename = detail::enable_if_t<!std::is_lvalue_reference<T>::value>>
     operator T&&()
     const;
@@ -1655,6 +1656,7 @@ namespace trompeloeil
 #endif

     template <typename T,
+             typename = detail::enable_if_t<!std::is_same<std::decay_t<T>, wildcard>::value>,
               typename = detail::enable_if_t<std::is_copy_constructible<T>::value
                                              || !std::is_move_constructible<T>::value>>
     operator T&()

I think there's a bug somewhere in the parameter matching logic, because I don't think it should ever try to convert a wildcard into a wildcard. That doesn't make sense. The comment about the needed constructor from anything for g++ 4.9 and 5 also hints in the same direction.

Unfortunately, with this work around, clang++-6 still ICEs on the `compiling_tests´ program.

I'm not pushing this, because I want to try and find the underlying problem instead. Then there's also the ICE which would be nice to reproduce in a minimal program and report to LLVM.

AndrewPaxie commented 6 years ago

Thanks for the tip about the awesome power of Compiler Explorer's #include mechanisms.

This was the smallest example I could find today before I read your notes. It doesn't involve Trompeloeil at all.

#include <tuple>
#include <type_traits>

struct wildcard
{
  template <typename T,
            typename = std::enable_if_t<std::is_copy_constructible<T>::value
                                        || !std::is_move_constructible<T>::value>>
  operator T&()
  const;
};

static constexpr wildcard const _{};

int
main()
{
  auto t = std::make_tuple(_, _);

  return 0;
}

I'll reread what you wrote. Perhaps you can correct this example.

AndrewPaxie commented 6 years ago

Also saw the ICEs that you observe. Sorry for not being precise.

rollbear commented 6 years ago

I don't think your new example is quite equivalent (and if it is, there in lies the clue for where the bug in the parameter matching logic lies.) std::make_tuple can match any type, so the wildcard is not guided. The parameter matching logic should try to see if the parameter type, that is known, can be constructed from a wildcard. This should never match a wildcard (unless some joker writes MAKE_MOCK1(foo, void(trompeleoeil::wildcard)), but that makes little sense in any real world program.

rollbear commented 6 years ago

Interesting, there seems to be more than one ICE. The below patch makes a completely different ICE appear.

diff --git a/test/compiling_tests_14.cpp b/test/compiling_tests_14.cpp
index 68e53cf..9467f1e 100644
--- a/test/compiling_tests_14.cpp
+++ b/test/compiling_tests_14.cpp
@@ -3434,29 +3434,36 @@ Tried obj\.foo\(not_empty\{\}\) at [A-Za-z0-9_ ./:\]*:[0-9]*.*

 #endif /* TROMPELOEIL_TEST_REGEX_FAILURES */

-auto is_clamped_lambda =
-  [](auto x, auto min, auto max) ->decltype(x >= min && x <= max)
+template <typename T>
+struct is_clamped_t
 {
-  return x >= min && x <= max;
+  template <typename U>
+  auto operator()(U x, T min, T max) const -> decltype(x >= min && x <= max)
+  {
+    return x >= min && x <= max;
+  }
 };

-template <typename kind = trompeloeil::wildcard, typename T>
-auto is_clamped(T min, T max)
+template <typename T>
+struct is_clamped_error_print
 {
-  using trompeloeil::make_matcher;
-  return make_matcher<kind>(
-    is_clamped_lambda,
-
-    [](std::ostream& os, auto amin, auto amax) {
+  void operator()(std::ostream& os, T amin, T amax) const {
       os << " in range [";
       ::trompeloeil::print(os, amin);
       os << ", ";
       ::trompeloeil::print(os, amax);
       os << "]";
-    },
-
-    min,
-    max);
+  }
+};
+template <typename kind = trompeloeil::wildcard, typename T>
+auto is_clamped(T min, T max)
+{
+  using trompeloeil::make_matcher;
+  return make_matcher<kind>(
+                           is_clamped_t<T>{},
+                           is_clamped_error_print<T>{},
+                           min,
+                           max);
 }

 TEST_CASE_METHOD(

I'll see if I can hunt this one down as a separate program to base a bug report on.

Some earlier versions of gcc had problems with generic lambdas in local scope, that's why the example has the lambda declared in global scope. That it works when one writes essentially the same class by hand, should be a nice clue for the LLVM developers.

rollbear commented 6 years ago

This is the closest I've come so far regarding a free standing program that triggers the clang ICE.

Restorinng the commented out enable_if:s makes it compile.

#include <type_traits>
#include <utility>
#include <tuple>

 template <typename T>
struct is_clamped_t
{
  template <typename U>
  auto operator()(U x, T const&min, T const& max) const -> decltype(x >= min && x <= max)
  {
    return x >= min && x <= max;
  }
};

  template <typename Pred, typename ... T>
  class duck_typed_matcher
  {
  public:
    template <typename V,
          //          typename = std::enable_if_t<!std::is_base_of<duck_typed_matcher, std::decay_t<V>>{}>,
              typename = decltype(std::declval<Pred&>()(std::declval<V&&>(), std::declval<T>()...))>
    operator V&&() const;

    template <typename V,
          //          typename = std::enable_if_t<!std::is_base_of<duck_typed_matcher, std::decay_t<V>>{}>,
              typename = decltype(std::declval<Pred&>()(std::declval<V&>(), std::declval<T>()...))>
    operator V&() const;
  };

template <typename Predicate, typename MatcherType, typename ... T>
  class predicate_matcher
    : private Predicate
    , public MatcherType
  {
  public:
    template <typename ... U>
    constexpr
    predicate_matcher(
      Predicate&& pred,
      U&& ... v)
    noexcept(noexcept(std::tuple<T...>(std::declval<U>()...)) && noexcept(Predicate(std::declval<Predicate>())))
      : Predicate(std::move(pred))
      , value(std::forward<U>(v)...)
    {}
  private:
    std::tuple<T...> value;
  };

template <typename Predicate, typename ... T>
inline
predicate_matcher<Predicate,
          duck_typed_matcher<Predicate, std::decay_t<T>...>,
          std::decay_t<T>...>
make_matcher(Predicate pred,  T&& ... t)
{
  return {std::move(pred),  std::forward<T>(t)...};
}

template <typename T>
auto is_clamped(T min, T max)
{
  return make_matcher(
              is_clamped_t<T>{},
              min,
              max);
}

 static_assert(std::is_convertible<decltype(is_clamped("a", "b")), decltype(is_clamped("a", "b"))>{},"foo");
int

main()
{
}

Unfortunately, adding this logic inside Trompeloeil does not make it work (although it does make the ICE go away.)

I still think something is broken with how Trompeloeil tries to match parameter types with matchers. It should never try to match a matcher with itself.

rollbear commented 6 years ago

The clang++-6 ICE is now reported:

https://bugs.llvm.org/show_bug.cgi?id=38010

Now to figure out a work around...

AndrewPaxie commented 6 years ago

Thanks. Since this might not be the last defect in Clang, I'll go ahead and get logins for the LLVM and GCC bugzillas.

rollbear commented 6 years ago

I may have it solved!

diff --git a/include/trompeloeil.hpp b/include/trompeloeil.hpp
index 1fbead3..03f36ed 100644
--- a/include/trompeloeil.hpp
+++ b/include/trompeloeil.hpp
@@ -850,7 +850,7 @@ namespace trompeloeil
   struct matcher { };

   template <typename T>
-  using matcher_access = decltype(static_cast<matcher*>(std::declval<typename std::add_
pointer<T>::type>()));
+  using matcher_access = decltype(static_cast<const matcher*>(std::declval<typename std
::add_pointer<std::decay_t<T>>::type>()));

   template <typename T>
   using is_matcher = typename is_detected<matcher_access, T>::type;
@@ -877,6 +877,9 @@ namespace trompeloeil
   };

   template <typename Pred, typename ... T>
+  using predicate_call = decltype(std::declval<const Pred&>()(std::declval<T>()...));
+
+  template <typename Pred, typename ... T>
   class duck_typed_matcher : public matcher
   {
   public:
@@ -886,13 +889,17 @@ namespace trompeloeil
     // g++ 4.8 gives a "conversion from <T> to <U> is ambiguous" error
     // if this operator is defined.
     template <typename V,
-              typename = decltype(std::declval<Pred>()(std::declval<V&&>(), std::declva
l<T>()...))>
+              typename = detail::enable_if_t<!is_matcher<V>{}>,
+              typename = predicate_call<Pred, V&&, T...>>
+//              typename = decltype(std::declval<const Pred&>()(std::declval<V&&>(), st
d::declval<T>()...))>
     operator V&&() const;

 #endif

     template <typename V,
-              typename = decltype(std::declval<Pred>()(std::declval<V&>(), std::declval
<T>()...))>
+              typename = detail::enable_if_t<!is_matcher<V>{}>,
+              typename = predicate_call<Pred, V&, T...>>
+//              typename = decltype(std::declval<const Pred&>()(std::declval<V&>(), std
::declval<T>()...))>
     operator V&() const;
   };

@@ -1621,6 +1628,12 @@ namespace trompeloeil
     }
   }

+    template <typename T>
+    using copy_type = decltype(T(std::declval<const T&>()));
+
+    template <typename T>
+    using move_type = decltype(T(std::declval<T>()));
+
   inline
   void
   sequence_type::add_last(
@@ -1645,16 +1658,15 @@ namespace trompeloeil

     // g++ 4.8 gives a "conversion from <T> to <U> is ambiguous" error
     // if this operator is defined.
-    template <typename T,
-              typename = detail::enable_if_t<!std::is_lvalue_reference<T>::value>>
+      template <typename T,
+                typename = detail::enable_if_t<!std::is_lvalue_reference<T>::value>>
     operator T&&()
     const;

 #endif

-    template <typename T,
-              typename = detail::enable_if_t<std::is_copy_constructible<T>::value
-                                             || !std::is_move_constructible<T>::value>>
+      template <typename T,
+                typename = detail::enable_if_t<is_detected<copy_type, T>{} || !is_detected<move_type, T>{}>>
     operator T&()
     const;

At least clang++-6 accepts it with -std=c++17 and the self test runs without error.

It needs cleaning up and check with other compilers. Opinions?

AndrewPaxie commented 6 years ago

Impressive! I've only had time to read the patch inline at the moment, but should be able to try the same techniques in some other small samples of code the produce others of the three ICEs that we have (about 12 hours from now, I afraid).

Would you spent a minute explaining typename = predicate_call<Pred, V&&, T...>>? I think I'm missing the context where the arguments Pred and T... are provided.

Wonderful to have tools like is_detected in our toolkit, a problem to solve, an inquiring mind, and an adventurous spirit!

rollbear commented 6 years ago

I wish I could claim credit for the detection idiom, but I'm just a lowly thief.

Watch Marshal Clow explain it: https://www.youtube.com/watch?v=U3jGdnRL3KI

As for predicate_call, watch where they're used. It should be shorthand for just inlining the expression, but to my surprise it's not (which kind of hints at a clang++ bug.)

I have a similar problematic construction in a plea for help on twitter. So far no serious takers, but who knows. The work around works, but I want to know if it's by luck gambling undefined behaviour, or whatever it is: https://twitter.com/bjorn_fahller/status/1013780524240957442

AndrewPaxie commented 6 years ago

Thanks for the link - tonight's viewing. This lightning came from Walter E. Brown, N3911, IIRC.

AndrewPaxie commented 6 years ago

Ok, I am thinking more clearly now.

Small quibble, in matcher_access, std::decay_t -> detail::decay_t.

Essentially three changes,

Made these changes to my copy of trompeloeil.hpp.

Ran

Enabled all tests in ./run_all_compiling_tests.sh

./run_all_check_errors.sh - OK ./run_all_compiling_tests.sh - only partial success, compiler errors for:

                Language    Standard
Compiler        Standard    Library     Comment
----------      --------    --------    -------
clang++-3.8     c++14       stdc++ v7
clang++-3.8     c++11       stdc++ v7

clang++-3.7     c++14       stdc++ v7
clang++-3.7     c++11       stdc++ v7

clang++-3.6     c++14       stdc++ v7
clang++-3.6     c++11       stdc++ v7

clang++-3.5     c++14       stdc++ v7
clang++-3.5     c++11       stdc++ v7

Partial dump of error messages from the compiler (clang++-3.8 -std=c++14 in this case).

./include/trompeloeil.hpp:1666:30: error: calling a protected constructor of class 'std::basic_ostream<char>'
  using move_type = decltype(T(std::declval<T>()));
                             ^

// :::

/usr/include/c++/7/ostream:393:7: note: declared protected here
      basic_ostream(basic_ostream&& __rhs)
      ^
test/compiling_tests_11.cpp:1169:38: error: no viable conversion from 'const trompeloeil::wildcard' to 'std::basic_ostream<char>'
    REQUIRE_CALL_V(u, func_streamref(_));
                                     ^
// :::

test/compiling_tests.hpp:297:3: note: passing argument to parameter 'p1' here
  MAKE_MOCK1(func_streamref, void(std::ostream&));

I mention the stdlibc++-v3 library as stdc++ v7 as a shorthand for the version of the library shipped with GCC 7.3.0. These compilers cannot compile with stdc++ v8 (= `libstdc++-v3 shipped with GCC 8.0.1): the library is incompatible due to use of builtins that are not implemented in these compilers.

Even though Marshall Clow talked about the detector idiom working with protected members, and that declval<T>() returns an rvalue-reference, it seems that these compilers complain when access checks fail in an unevaluated context.

Trompeleoil's detector idiom seems to be a proper subset of what's outlined in the the video.

Will report on how MSVC 2015 and 2017 treats this new code.

AndrewPaxie commented 6 years ago

Microsoft Visual Studio 2017 Community 15.7.4 results

rollbear commented 6 years ago

I tested with g++-4.8 -std=c++11, and noticed the std::decay_t -> detail::decay_t

copy_type and move_type should be named better. Also predicate_call should be renamed, since it gives the resulting type of a call. Similar to (now deprecated) std::result_of. There are a few places in the code where this kind of evaluation takes place, and they should all use the renamed predicate_call.

Great about MSVC 2017. Fingers crossed for MSVC 2015 then.

rollbear commented 6 years ago

I think a good renaming is predicate_call -> detail::invoke_result_t, which can use (at least with clang++-6, I checked) std::invoke_result_t in C++17.

AndrewPaxie commented 6 years ago

Decided to try this platform setting in VS 2017:

Platform Toolset: Visual Studio 2015 (v140)

as a proxy for a "genuine" Visual Studio 2015 development environment.

An encouraging sign was receiving this warning: D9002: ignoring unknown option '/permissive-'.

(Some other warnings have been omitted from this report.)

Results:

So really what are left with is some fallback strategy for the soon-to-be renamed move_type for the earlier versions of Clang.

AndrewPaxie commented 6 years ago

Do you know why std::result_of was deprecated? Some change in semantics that could only be expressed in a new function name?

rollbear commented 6 years ago

https://en.cppreference.com/w/cpp/types/result_of#Notes

rollbear commented 6 years ago

We can implement our own is_copy_constructible<T> and is_move_constructible<T>, which uses the std:: implementations where it works, and the detection fallback for others. Even more ugly #ifdef, but what to do? I'd prefer it if we could find one solution that works for all compilers, but I currently doubt that we can.

rollbear commented 6 years ago

Added another clang bug report: https://bugs.llvm.org/show_bug.cgi?id=38033

It's so weird that uncommenting the static_assert makes it compile.

https://godbolt.org/g/3vELTM

AndrewPaxie commented 6 years ago

Did some digging and found this interesting comment in libc++, /usr/include/c++/v1/type_traits in struct __is_constructible_helper

    // This overload is needed to work around a Clang bug that disallows
    // static_cast<T&&>(e) for non-reference-compatible types.
    // Example: static_cast<int&&>(declval<double>());
    // NOTE: The static_cast implementation below is required to support
    //  classes with explicit conversion operators.

I think wildcard has "explicit conversion operators".

It also hints that the implementation of is_constructible in libc++ is very finely wrought.

Corresponding commit

https://github.com/llvm-mirror/libcxx/commit/2c6c2b944e85178a3dcc8012dc43496f317c5fa0

Still trying to tie this to a defect report in bugs.llvm.org.

rollbear commented 6 years ago

Hmmm... not so sure, but I guess English mixed with standardeese can be a bit ambiguous at times.

Does "explicit" above mean that there is an explicitly written conversion operator, then, yes, it has one. Does "explicit" mean that the conversion operator is marked "explicit" to avoid implicit conversions, then no, it does not have one.

AndrewPaxie commented 6 years ago

I agree with your observation of the ambiguity of the phrases involved. The same thoughts occurred to me the more time went on.

The code in <type_traits> is fallback code used when the compiler doesn't have a builtin is_constructible (if I understand what !__has_feature(is_constuctible) means (around line 3022)).

Looks like time to examine the history of this in the Clang source.

AndrewPaxie commented 6 years ago

I have fallen down the rabbit hole, which starts at lib/Sema/SemaExprCXX.cpp/evaluateTypeTrait(). Oh my, looks like I won't be productive for a while.

AndrewPaxie commented 6 years ago

I may have another workaround, one that doesn't require replacements for is_copy_constructible<T> and is_move_constructible<T>, but guards them instead. Code just for operator in wildcard to illustrate

    template <
      typename T,
      typename = detail::enable_if_t<
        !std::is_base_of<
          wildcard,
          detail::decay_t<T>
        >::value
      >,
      typename = detail::enable_if_t<
        std::is_copy_constructible<T>::value
        || !std::is_move_constructible<T>::value
      >
    >
    operator T&()
    const;

This directly implements your observation that we shouldn't be enabling this operator for wildcard.

This is the same guard test that we were advised to use to resolve the clang-tidy warning bugprone-forwarding-reference-overload.

Compiled without error for clang++-6.0 -std=c++17 with libstdc++-v3.

Regression testing with other build configurations now.

rollbear commented 6 years ago

This is good news, but I'm surprised that you're not seeing problems with duck_typed_matcher?

AndrewPaxie commented 6 years ago

I just changed the guard for operator T&() const in wildcard. Changes for duck_typed_matcher are the ones you proposed that use predicate_call.

There may be a smaller set of changes once this one is validated.

Status so far: All GCC build configurations passed. On to Clang builds. MSVC is being done by hand.

AndrewPaxie commented 6 years ago

All Clang build configurations pass, as do those for Visual Studio 2017 and Visual Studio 2015.

rollbear commented 6 years ago

Excellent! I'm not very worried about the gcc builds, but must obviously wait for the results.

So then it's just a matter of bike-shedding the names then ;-)

AndrewPaxie commented 6 years ago

I experimented with enabling operator T&&() const in wildcard for g++-4.8 but I couldn't work an appropriate constraint for it to avoid an error, conversion from 'wildcard' to 'int' is ambiguous.

So should I create a pull request for the progress we have achieved for you to review, merging your work in duck_typed_matcher with the tweaks I have made to wildcard?

rollbear commented 6 years ago

You can do this, if you want to, otherwise I had planned to do add the fixes on a topic branch today. With that in place, the travis-ci builds should also be widened to include -std=c++17 builds for the compilers that support it.

AndrewPaxie commented 6 years ago

May I leave it to you, since you made the breakthroughs?

After we merge this and pull request #93, the Travis (and AppVeyor) extensions should be a breeze.

rollbear commented 6 years ago

Absolutely. I'm on it.

rollbear commented 6 years ago

Can you have a look at topic branch https://github.com/rollbear/trompeloeil/tree/clang_38010_38033 @AndrewPaxie ?

If you're happy with the code, just merge it to master and delete the branch.

I'm not entirely happy with the duplicated using can_copy_construct..., but not annoyed enough to work out the proper logic.

AndrewPaxie commented 6 years ago

Sorry for the delays - had an interesting problem building g++-latest as a side-effect of P0935R0 being applied. A "rebuild all" fixed the missing std::ostringstream default constructor.

I uncommented the constraint code on wildcard::operator T&&() const that was left in and observed the warning -Wfloat-equal from Clang compilers when compiling in exactly -std=c++17 mode. Seems like some issue with the implementation of std::invoke_result_t, yet it affects both libstdc++-v3 and libc++. Perhaps another case of "you're using it wrong", but I will not investigate further. Instead, I will remove this code and retest, since this was the configuation in which you reported success.

BTW: Visual Studio 2017 is now at 15.7.5 which is what I will be testing with.

I also found this interesting historical problem report for GCC, Bug 64385 " - odd behaviour of std::is_move_constructible< std::ostringstream >" Jonathan Wakely's workaround seems worth keeping in mind.

rollbear commented 6 years ago

Yes, I also got the -Wfloat-equal from clang. I don't understand it at all. There are no floating point comparisons where it warns. Another bug to try to find a minimal demonstration for, maybe?

AndrewPaxie commented 6 years ago

Unfortunately I get compile errors from clang++-6.0 -std=c++2a with libstdc++-v3. Once more it's - error: base class has incomplete type. AND the -Wfloat-equal warning in decltype(x op y) remains.

Did you last commit end up expressing a different intent?

I am going to look at the confusion tomorrow when I am fresh. Perhaps you can see what I cannot.

rollbear commented 6 years ago

I can only conclude that I must've missed that one in my tests. It fails for -std=c++17 too when using libstdc++. But since it's complaining about std::is_constructible<>, it means it's missing my home brew detection-based can_construct<>. Seems I was right to do this job on a topic branch.

AndrewPaxie commented 6 years ago

I'm inlining a patch here as a starting point for where this topic branch is likely to go. Probably some cleanup to be followed through on. Features

(1) Defines a TROMPELOEIL_CLANG_VERSION macro as we (probably) need to compare versions of the Clang compiler now. (2) Abandons using std::invoke_result_t - the -Wfloat-equals warning is a separate investigation. (3) Removes !is_base_of constraint.

Compiled without errors or warnings and passed unit tests on all GCC and Clang versions.

If you've already worked out another solution, then that's OK.

I noticed that Compiler Explorer now has Visual Studio 2015, 2017, and 2018 Preview support. Not sure how much support, but a few random tests were encouraging.

From what I've read about the Visual Studio 2017 15.8 Preview 3 preprocessor changes, the /permissive- flag is going to have to part of the build confguration. I have a good sketch of the GCC and Clang evolution and will work on the Visual Studio build configurations over the next couple of days. Want to get this issue out of the way first.

Patch to branch clang_38010_38033

diff --git a/include/trompeloeil.hpp b/include/trompeloeil.hpp
index e2de086..2dd2ba7 100644
--- a/include/trompeloeil.hpp
+++ b/include/trompeloeil.hpp
@@ -51,6 +51,9 @@
 #  define TROMPELOEIL_GCC 0
 #  define TROMPELOEIL_MSVC 0

+#  define TROMPELOEIL_CLANG_VERSION \
+  (__clang_major__ * 10000 + __clang_minor__ * 100 + __clang_patchlevel__)
+
 #  define TROMPELOEIL_GCC_VERSION 0

 #  define TROMPELOEIL_CPLUSPLUS __cplusplus
@@ -61,6 +64,7 @@
 #  define TROMPELOEIL_GCC 1
 #  define TROMPELOEIL_MSVC 0

+#  define TROMPELOEIL_CLANG_VERSION 0
 #  define TROMPELOEIL_GCC_VERSION \
   (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__)

@@ -72,6 +76,7 @@
 #  define TROMPELOEIL_GCC 0
 #  define TROMPELOEIL_MSVC 1

+#  define TROMPELOEIL_CLANG_VERSION 0
 #  define TROMPELOEIL_GCC_VERSION 0

 #  if defined(_MSVC_LANG)
@@ -602,8 +607,9 @@ namespace trompeloeil

 # endif /* !(TROMPELOEIL_CPLUSPLUS == 201103L) */

-# if TROMPELOEIL_CPLUSPLUS == 201703L
-#   if TROMPELOEIL_CLANG
+# if TROMPELOEIL_CPLUSPLUS >= 201703L
+#   if TROMPELOEIL_CLANG && TROMPELOEIL_CLANG_VERSION >= 60000
+
   // these are mostly added to work around clang++ bugs
   // https://bugs.llvm.org/show_bug.cgi?id=38033
   // https://bugs.llvm.org/show_bug.cgi?id=38010
@@ -628,8 +634,10 @@ namespace trompeloeil
   using can_copy_construct = std::is_copy_constructible<T>;
 #   endif

+  //template <typename F, typename ... A>
+  //using invoke_result_type = std::invoke_result_t<F, A...>;
   template <typename F, typename ... A>
-  using invoke_result_type = std::invoke_result_t<F, A...>;
+  using invoke_result_type = decltype(std::declval<F&>()(std::declval<A>()...));

 # else

@@ -936,14 +944,6 @@ namespace trompeloeil

     template <
       typename T,
-#if 0
-      typename = detail::enable_if_t<
-        !std::is_base_of<
-          wildcard,
-          detail::decay_t<T>
-        >::value
-      >,
-#endif
       typename = detail::enable_if_t<
         can_copy_construct<T>::value
         || !can_move_construct<T>::value
rollbear commented 6 years ago

Thank you, that does look like it works. I've pushed that to the topic branch, mostly to trigger a rerun of travis-ci and appveyor. If it passes, which I expect it to, I'll merge to master and kill the topic branch.

rollbear commented 6 years ago

That worked fine, so merged now. Phiew.