search

rollbear / trompeloeil

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

Lower requirements to C++11? #1

Closed SimonKagstrom closed 9 years ago

SimonKagstrom commented 9 years ago

Would it be much work to reduce the compiler requirements to C++11 instead? E.g., the Ubuntu 12.04 compilers does not (as far as I know) support C++14, which makes travis.org builds difficult.

rollbear commented 9 years ago

The one needed feature of C++14 is auto in lambda. It is probably possible to work around, but it's not trivial (I have tried, although not very hard, and failed.) This is for the parameter to the lambda in the *_(capture, ...) macros. Since the type is known in call_modifier<> template, which the lambda is given to, it should be possible to grab the type from there, but I've so far failed to do so.

SimonKagstrom commented 9 years ago

OK, I understand. I guess the travis-case can be worked around by installing a newer compiler from some ppa before building the unit tests.

SimonKagstrom commented 9 years ago

Ouch, so GCC 4.9 is required? Unfortunately even Fedora 20 won't cut it then.

Perhaps a clever macro solution can be used instead ;-)

rollbear commented 9 years ago

Ugh, I'm sure some clever macro solution can be used. I'm trying to reduce the frequency of those, not increase it...

clang 3.5 works fine too.

rollbear commented 9 years ago

I'm closing this now. Perhaps I, or someone else, comes up with a clever solution, in which case it'll be incorporated, but chances are c++14 will become mainstream before that.

AndrewPaxie commented 7 years ago

I am willing to work on backporting Trompeloeil to C++11. A couple of hours work has resolved the trivial issues (type trait helpers, exchange, integer_sequence) and am ready to attempt the generic lambdas and decltype(auto). I don't really want to maintain a fork "forever". Is there enough interest to accept a contribution on this issue back to the project? (I will only be targeting GCC 4.8.3 in the first instance.)

rollbear commented 7 years ago

decltype(auto) is probably possible to work around, but I think you may find generic lambdas to be the killer. When I first started on this project, C++11 was on my mind, but I couldn't figure out a way around generic lambdas. That said, my inability to find a solution doesn't mean that none exists.

I have mixed feelings about this, though. On the one hand I find your enthusiasm commendable, and the chance for widened support a very good thing indeed. On the other hand, I'm have my attention in the other temporal direction, and try to see what of C++17 I can take advantage of (as possible extensions) without sacrificing C++14 support.

AndrewPaxie commented 7 years ago

Yes, my enthusiasm for this issue will never be higher than now. I remain undeterred and will proceed.

I share your desire to look forward instead of backward. Perhaps the following strategy could allay you.

Let's say the best possible outcome actually occurs: I succeed with a C++11 backport, the code review is passed, and you merge the pull request into this project. In the release that you announce (limited) support for C++11 you also immediately deprecate its support. You remain free to drop C++11 support at the time of your choosing, in particular at the time you decide to add a useful C++17-based feature that impacts a C++11-sensitive code path.

My needs are short-term (effectively one release only). I hope anyone who needs C++11 support now is using it as a stepping stone release to C++14.

Of course, if I don't succeed, then I will add this to my list of heroic failures and the set of Those-Who-Tried-and-Failed will have increased by one. We form a club, have a whisky someday, maybe the next ACCU conference, and drink a toast to "hard problems without solutions (so far)." No hard feelings.

rollbear commented 7 years ago

;-)

You may want to have a look at the "detector" branch and see if the detection idiom is working with g++ 4.8 -std=c++11. I intend to merge that into "develop" very soon, since it has no negative impact on compilation time, and it makes the source easier to read.

AndrewPaxie commented 7 years ago

Progress Report

Good news, so far. I started from the detector branch and have code that compiles clean with g++-4.8 -std=c++11 (GCC 4.8.5). No issues so far with void_t and related detector code. All generic lambdas have been lowered to function objects. decltype(auto) has its own backward compatibility macros. Yes, macros, but only two. On to testing, ChangeLog and administration.

I'm not very familiar with the process of creating a branch and getting it reviewed. Please advise the steps to take to get this code into the repo in a way that meets the project guidelines.

rollbear commented 7 years ago

This sounds exciting indeed. I'm very curious how you've managed to replace the generic lambdas with function objects in .RETURN(), .THROW(), .WITH() and .SIDE_EFFECT().

For tests, get compiling_tests.cpp through and you should be more or less set. It needs Catch! I believe the tests for regex will fail, so those probably need to be #ifdef:ed on c++ version. Maybe some others too.

It's also highly desirable to get the compilation error tests through. CXX=g++-4.8.5 ./check_errors.sh You probably need to alter the script since it explicitly sets -std=c++14.

This weekend I killed the detector branch after having merged it into develop, To move on, please get a nice set of commits for the functionality on the develop branch, and issue a pull request. I'd appreciate an updated .travis.yml to be included, since otherwise I'd be sure to accidentally break g++-4.8 at any time.

AndrewPaxie commented 7 years ago

I think I understand, thanks for the pointers. Will switch to develop branch and take it from there.

Once code is proven working, I'll blog about the techniques used.

Update: OOPS - missed the content of the macros .RETURN(), .THROW(), .WITH() and .SIDE_EFFECT()! Of course this came to light with the first run of compiling_tests.cpp. Well, that's where the real work is then.

AndrewPaxie commented 7 years ago

Progress report

Positive news, of a sort.

New interface macros C++11 support

I have a version of Trompeloeil with alternative macros for .RETURN(), .THROW(), .WITH(), and .SIDE_EFFECT() and their LR_ siblings that compiles with g++-4.8 -std=c++11. These macros take two extra arguments, obj and func and I have called them .WITH_MOCK_ARGS() etc. for want of a better name (do you have a better suffix?).

The extra arguments are used to extract the type of trompeloeil_x, which is always call_params_type_t<sig>, as follows

#define TROMPELOEIL_WITH_MOCK_ARGS_(capture, obj, func, arg_s, ...)            \
  with(                                                                        \
    arg_s,                                                                     \
    [capture](                                                                 \
      decltype(                                                                \
        (obj).TROMPELOEIL_CONCAT(trompeloeil_tag_,func)                        \
      )::call_params_type_t const&                                             \
      trompeloeil_x)                                                           \
    {                                                                          \
      :::
    })

call_params_type_t is a using declaration in struct TROMPELOEIL_LINE_ID(tag_type_trompeloeil) defined as

using call_params_type_t = ::trompeloeil::call_params_type_t<sig>;

when expanding the macro TROMPELOEIL_MAKE_MOCK_().

I tried hard to keep the existing interface, but have to this point been unable to extract the required type information from the context in which the lambda is evaluated. Maintaining the elegance of the fluent interface for defining expectations was a concern of mine. I am most interested if you (or anyone!) finds a way to communicate the type for trompeloeil_x across the code generated in the REQUIRE_CALL() macro (and siblings) and .WITH() etc.

These new macros also work in C++14 mode so there is a mechanical, incremental, migration path to the current API when g++ 4.8.x is finally laid to rest.

Unit tests are work-in-progress

I am working through compiling_tests.cpp, duplicating the test cases and changing the copies to use the new macros. All original test cases run in C++14 mode, so nothing has been lost.

G++ 4.8.5 has incomplete support for <regex>, so for the moment test cases that fail because of this are conditionally enabled only for G++ 4.9.0 or above. I will revisit them after my first pass through the file and see what can be done. Clang support is maintained. Microsoft compilers are as yet untested.

Onward!

rollbear commented 7 years ago

Yes, you've reached exactly the same stumbling block as I did when I tried, although I gave up where you persist.

I'm trying to think of a way to get the types across. If I'm successful, I'll let you know, although in honesty I'm not optimistic.

rollbear commented 7 years ago

I have an idea. I'll do some experiments if I find the time, but this week is pretty nutty, so no promises.

The outline, though, is that there is a type template:

template <unsigned long N>
struct param_type_access
{
  using type = typename param_type_access<N-1>::type;
};

template <>
struct param_type_access<0>
{
};

REQUIRE_CALL() (and siblings) make a specialization:

template <>
struct param_type_access<__LINE__>
{
  using type = call_params_type_t;
};

And then .WITH() etc. access the type via param_type_access<__LINE__>::type, which should find the specialization on the same line, or the next few lines above.

This will obviously break apart if you have several expectations on the same line, but the idea probably works, albeit with a measurable penalty in compilation time.

AndrewPaxie commented 7 years ago

Genius. I added a partial implementation to try the idea out

Compiled compiling_tests.cpp with this change and immediately hit this error

trompeloeil.hpp:3912:3: error: a template declaration cannot appear at block scope
   template <>                                                                  \
   ^
trompeloeil.hpp:3909:3: note: in expansion of macro ‘TROMPELOEIL_REQUIRE_CALL_’

Unfortunately it appears we cannot remedy the situation using this technique directly.

I gave some thought to having a macro to declare the necessary machinery at namespace scope, e.g.

#define TROMPELOEIL_PARAM_TYPE_ACCESS(obj, func)                               \
  template <>                                                                  \
  struct param_type_access<__LINE__>                                           \
  {                                                                            \
    using type =                                                               \
      decltype(                                                                \
        (obj).TROMPELOEIL_CONCAT(trompeloeil_tag_,func)                        \
      )::call_params_type_t;                                                   \ 
  };

but all uses of obj, func have to be made before introducing another param_type_access specialization using the macro, else the modifiers will receive the wrong type when they perform their lookups. For example, this fails

TROMPELOEIL_PARAM_TYPE_ACCESS(obj, getter(ANY(unmovable&)))
TROMPELOEIL_PARAM_TYPE_ACCESS(obj, getter(_, _))
void foo()
{
    REQUIRE_CALL(obj, getter(ANY(unmovable&)))
      .RETURN(_1); // wrong call_param_type_t returned
    REQUIRE_CALL(obj, getter(_, _))
      .SIDE_EFFECT(_2 = std::to_string(_1));
}
rollbear commented 7 years ago

Yes, i thought about that during lunch and was just about to retract the suggestion.

I think something along those lines may be a way forward though. Perhaps defining a variable that you can look up and get a type from? The trick is, of course, to find the name without causing collisions when you have several expectations in the same scope.

AndrewPaxie commented 7 years ago

Progress report

More progress, I suppose. Since a reliable method of communicating type information between macros has not been found to date, I decided to make the mock type information explicit and move on.

Revised C++11 macros

I changed the surface syntax macros for C++11 to the following short names, (along with their LR_ variants):

NAMED_MOCK_TYPE(obj, func)      *new*
WITH_TYPE(mtype, ...)
SIDE_EFFECT_TYPE(mtype, ...)
RETURN_TYPE(mtype, ...)
THROW_TYPE(mtype, ...)

where the new macro NAMED_MOCK_TYPE() is just

#define TROMPELOEIL_NAMED_MOCK_TYPE(obj, func)                                 \
  decltype((obj).TROMPELOEIL_CONCAT(trompeloeil_tag_,func))

To use it, write it in a type alias declaration,

using m_t = NAMED_MOCK_TYPE(obj, getter(ANY(int)));

and pass the type alias name as the first argument to the new macros

REQUIRE_CALL(obj, getter(ANY(int)))
  .RETURN_TYPE(m_t, 1)
  .IN_SEQUENCE(seq);

By leaving it to the developer to provide the type alias name, multiple type names are supported in the same scope, one for each different expectation.

When migrating to C++14 and beyond, the transformation of removing the alias declaration and replacing '_TYPE(<mtype>,' with '(' for the matching macros is all that's required.

Unit tests

All unit tests in compiling_tests.cpp have been modified to work with both C++11 and C++14, though some are conditionally enabled, depending on compiler mode. All C++11 tests are run in C++11 mode (-std=c++11) and C++14 mode (-std=c++14), while C++14 tests are run in C++14 mode or later.

Each unit test is now tagged as [C++11][C++14] or [C++14] in addition to their previous tags.

check_errors.sh

I have performed a few runs with this script and am examining the regressions compared to the baseline the original source provides.

travis

Not quite ready to bring this online.

Known issues

<regex>

The problems with executing regular expressions is mostly due to libstdc++. I have had much more success with libc++ (Ubuntu package libc++1 3.9.1-2) but currently have been unable to link this library with code compiled with g++-4.8 -std=c++11. Command lines that work with every other version of g++ fail with this version with undefined references.

duck_typed_matcher and wildcard

g++-4.8 -std=c++11 gives "conversion is ambiguous" in duck_typed_matcher and wildcard if both opertor V&&() const and operator V&() const are defined at the same time. I have had to disable V&&() and provide explicit conversions for necessary types used in the unit tests. For example,

operator std::string&&() const

template <typename T>
operator std::unique_ptr<T>&&() const;

operator int&&() const;

This is a serious compiler bug with respect to Trompeloeil and really limits its general utility when used with g++-4.8. However, specific measures can be put in place to workaround this deficiency for a fixed codebase.

neg_matcher

None of the neg_matcher test cases are enabled at present, due to conversion problems with duck_typed_matcher. Using a disambiguated function template e.g. !trompeloeil::eq<std::string>("foo") is a workaround for this defect.

Conclusion and future work

Most (90%?) of the functionality of Trompeloeil works in C++11 mode. This is good enough for my continued effort to complete the outstanding work prior to a commit for review.

In addition to the work implied by the above report, I want to reduce the turnaround time on compiling compiling_tests.cpp. The file is now over 10500 lines and stressing out CLion (sorry JetBrains).

If this project makes it to production then I anticipate compiling the C++11 test cases a very few times while iterating heavily on C++14 and beyond code. This suggests using the method suggested in the Catch! documentation to separate the main from the tests and further having a file for each of the compilation modes.

I have the opportunity to compile the unit tests with Visual Studio 2017, so will try that before contemplating Travis.

The journey continues.

rollbear commented 7 years ago

This looks promising. Even without your additions, CLion is having a really hard time. It's my preferred dev environment, but not when working with Trompeloeil. I hope it'll catch up soon.

I do wonder, though, how do you feel about the usability with the new _TYPE macros? I feel that much of the simplistic elegance of use is lost with that quite repetitive code. Is that a price you think is worth paying when doing development in C++11?

AndrewPaxie commented 7 years ago

I have only had a few minutes to consider the "price" of the _TYPE macros. At the present time they are the difference between using Trompeloeil with a C++11 toolchain and having to use a different mock object framework. Therefore I am prepared to pay such a price.

As far as usability is concerned, yes, the goal was to have the elegance along with utility, but "C++14 changes everything" as Louis Dionne said. I regard this work as a step towards this goal and will continue to strive for its attainment.

In the meantime, there are four of the compilation_errors test cases that fail with C++11 mode. The cause is the way I specify the return type of the lambda in the RETURN_TYPE macro. What should be decltype(auto) is approximated with mtype::return_of_t which is too eager.

I am committing this work to branch c++11 (hope I can create branches) which is not ready for review but is ready for use.

AndrewPaxie commented 7 years ago

OK, can't commit to a branch directly. Advice?

rollbear commented 7 years ago

I created a c++11 branch from head on develop. I guess that if you get your fork in sync with my tree, you should be able to work from there.

AndrewPaxie commented 7 years ago

Fork is up-to-date (actually, with v27 of master) and have pushed a c++11 branch there. Will continue resolving regressions and getting the branch ready for review.

rollbear commented 7 years ago

I don't know if you saw my bugfix on branch develop just now, but it may have an impact on your work.

AndrewPaxie commented 7 years ago

Thanks for the heads up on the changes. Merged and tested the multiple inheritance fix (Issue #46) on g++-4.8.5 with success. Removed tabs.

I have started piloting the c++11 branch from 5 July. The saddest part was finding g++ 4.8.3 has a defect in <tuple> which prevents Trompeloeil compiling. Fixed in 4.8.4. See: bugzilla, "Bug 61947 - [4.8/4.9 Regression] Ambiguous calls when constructing std::tuple" Available: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61947

A couple of test cases are also disabled with parsing problems, also fixed in either 4.8.4 or 4.8.5.

Also found that libc++ 3.9.1-2 on clang++-3.5 produces an ICE when compiling compiling_tests.cpp. This will not be addressed.

Work continues to broaden the compiler platforms and get this branch ready for Travis.

rollbear commented 7 years ago

Another heads-up for you.

On branch develop I've just made major additions to CMakeLists.txt and .travis.yml (which now uses cmake.)

A part in that change is also a change in the directory structure to a more traditional one.

I plan on merging this to master for release once I've gotten conan to work again (I did not keep up when conan moved to bintray.)

AndrewPaxie commented 7 years ago

Progress report

Fixes

Tested the c++11 branch on Visual Studio 2017 Community Edition and resolved an error in my backport of re(). Many more unit tests are enabled as a result.

Introduced a macro, TROMPELOEIL_CPLUSPLUS, to cope with Visual C++ setting _cplusplus == 199711L.

Removed warnings from compiling_tests.cpp by compiling with the same command line flags used for Travis.

Tested with GCC 7.1.1 (Fedora 26) - pass.

Research on decltype(auto)

Tried to improve the number of passes for check_errors.sh. At present four tests fail in C++11 mode:

return_value_to_ref.cpp
return_wrong_pointer_constness.cpp
return_wrong_reference_constness.cpp
return_wrong_type_first.cpp

where fail means the desired static_assert is not triggered.

After some research, thought and experimentation I conclude that it is not possible to replace decltype(auto) in the trailing return type of the generic lambda in the macro RETURN() with nothing (implying auto deduction of the return type), and certainly not with decltype(__VA_ARGS__). Reworking the placeholder system is out-of-scope of this issue.

The closest I have come is as it stands now, -> mtype::return_of_t, which forces the check too early in the compilation.

Therefore I don't expect any further improvement in the number of passes of check_errors.sh for C++11 mode in the near term. All tests continue to pass in C++14 mode.

Next steps

Merge latest from develop.

Modify .travis.yml to broaden C++ modes used. I am considering adding stdc++ library support to the matrix of tests as well.

I have a couple of scripts, run_all_compiling_tests.sh and run_all_check_errors.sh, that "mimics" the work of Travis, but locally. Good for regression testing before a push, and perhaps useful to others. Should these scripts be committed?

Not considered at present is measuring the performance of compilation using the Trompeloeil header. How do you measure this? I would like this branch to have some relation to the performance of master but don't have a baseline measure for comparison.

rollbear commented 7 years ago

Hold on with the .travis.yml changes for a day or two. I have received a pull request for some much desired changes to it (https://github.com/rollbear/trompeloeil/pull/47) but I have some questions I want answered before accepting it.

I don't think you need to add your scripts. The reworked CMakeLists.txt (which works nicely from CLion, by the way,) is what is used from the updated .travis.yml too. It doesn't have check_errors.sh as a target, but that could be added.

I must admit that my compilation time measurements have all been rather ad-hoc. Whenever I have an idea for a change that may improve compilation time, I make a base line measurement from compiling_tests.cpp with various compilers, make the change, and rerun the measurement. It would be a good thing to have a system that consistently measures the build times and stores the results so they can be visualised, but maybe that would be over zealous.

rollbear commented 7 years ago

The .travis.yml file stuff is done now. Some ugly work-arounds, but it works. The CMake stuff is looking quite good too. I don't expect any major changes to these any time soon, so no need to hold back any longer.

rollbear commented 7 years ago

I forgot to mention that the provided CMakeLists.txt doesn't work with the CMake that is bundled with CLion. See https://youtrack.jetbrains.com/issue/CPP-8477

Using an external CMake works fine.

AndrewPaxie commented 7 years ago

Progress Report

Travis builds

It seems the branch c++11 has survived "trial by Travis" and now has a g++-4.8 -std=c++11 job that passes. See for example, \ https://travis-ci.org/AndrewPaxie/trompeloeil/builds/261471592

Split test/compiling_tests.cpp

I have also split test/compiling_tests.cpp into these files

$ git add compiling_tests.cpp
$ git add compiling_tests.hpp
$ git add compiling_tests_11.cpp
$ git add compiling_tests_14.cpp

and updated CMakeLists.txt accordingly. It may help build times in Travis to add -j 3 to the make VERBOSE=1 self_test command.

Locally, I have tracked down and installed clang-3.6 and an experimental version of gcc-7 so now have all supported compilers on Ubuntu and Windows at my fingertips.

New C++11 API

I have had time to consider the modifications made to the C++14 macros in order to get some progress with the C++11 backport. Recall that as it stands the WITH, SIDE_EFFECT, RETURN, and THROW macros were modified to take an extra argument, the "mock type" (more correctly, the expectation type).

For example,

    // C++ 14 expectation
    REQUIRE_CALL(obj1, count())
      .IN_SEQUENCE(seq)
      .RETURN(1);

is transformed to

    // C++11 expectation, version one
    using m_t = NAMED_MOCK_TYPE(obj1, count());
    REQUIRE_CALL(obj1, count())
      .IN_SEQUENCE(seq)
      .RETURN_TYPE(m_t, 1);

This was dissatisfying from the point of view of having to retype the expectation type as an argument in each macro that needs it. More serious is that all existing Trompeloeil documentation has to be reinterpreted when used in a C++11 context.

I still haven't found a solution to being able to convey the expectation type from the e.g. REQUIRE_CALL to the e.g. RETURN macro as an implicit argument. I believe the following approach however is a better compromise compared to C++11 API version one.

Instead of an implicit argument, use an explicit argument, but this time expressed as a macro:

    // C++11 expectation, version two
#   define TROMPELOEIL_EXPECTATION_ARGS obj1, count()    
    REQUIRE_CALL(obj1, count())
      .IN_SEQUENCE(seq)
      .RETURN(1);
#   undef TROMPELOEIL_EXPECTATION_ARGS

The advantages of this approach, which I am calling C++11 API version two, are:

The principal disadvantage is, of course, having to type a #define and #undef pair for each expectation, even though at first glance the information needed is already presented in the arguments to the expectation.

There may be a way to tune this even further, perhaps with additional macros to capture the essence of what's required:

    TROMPELOEIL_START_EXPECTATION(obj, count())
    REQUIRE_CALL(obj1, count())
      .IN_SEQUENCE(seq)
      .RETURN(1);
    TROMPELOEIL_END_EXPECTATION

Onward to the C++11 API, version two, and documentation drawn from the approximately 13,000 lines of working notes I have maintained during the prosecution of this issue.

rollbear commented 7 years ago

This gives me an idea.

Instead of a START/END pair, you could probably make a wrapper macros, something like:

#define REQUIRE_CALL11(obj, func, ...) \
  using decltype(TROMPELOEIL_LINE_ID(ret_type)) = decltype((obj).func); \
  TROMPELOIEIL_REQUIRE_CALL(obj, func), __VA_ARGS__)

You'd then have C++11 versions of RETURN() etc, that makes use of TROMPELOEIL_LINE_ID(ret_type).

Usage would look like:

  REQUIRE_CALL11(obj, func(),
               RETURN11(3)
  );

This should work because the expansion of the REQUIRE_CALL11 macro will include the RETURN11() macro, with the same __LINE__, so they see the same ret_type.

The naming is not a suggestion, btw, I think it's appalling, I just wanted to highlight what would need to change.

AndrewPaxie commented 7 years ago

I had a similar idea after seeing my first START/END macro attempts fail miserably. I will follow this line and see where it leads.

AndrewPaxie commented 7 years ago

Progress report

C++11 version two API

I have completed a cut of the C++11 version two API that is a balance between the macro form you suggested and the START/END macros where the design started.

I created a single MAKE_EXPECTATION macro to wrap around an expectation,

#define TROMPELOEIL_MAKE_EXPECTATION(obj, func, ...)                           \
  using TROMPELOEIL_LINE_ID(e_t) =                                             \
    decltype((obj).TROMPELOEIL_CONCAT(trompeloeil_tag_,func));                 \
  __VA_ARGS__

Example,

MAKE_EXPECTATION(obj1, count(),
REQUIRE_CALL(obj1, count())
  .IN_SEQUENCE(seq)
  .RETURN(1)
);

The WITH, RETURN, SIDE_EFFECT, and THROW macros reference TROMPELOEIL_LINE_ID(e_t) from which call_params_type_t and return_of_t may be accessed.

The reason I didn't place the using declaration within the REQUIRE_CALL is because there is not always a call modifier to the call. This means the type introduced by the using may, in fact, go unused, leading to the warning -Wunused-local-typedefs from GCC when -Wall is used.

This retains a smooth migration path to C++14 API, but has the downside of requiring multiple mentions of the obj, func pair.

This is a statement-oriented implementation. One cannot place a TROMPELOEIL_MAKE_EXPECTATION macro around an expectation used as a function parameter, default argument, member of an initializer list, member initialization in a constructor, or any other place where an expression is required.

For code such as,

    e[0] = NAMED_REQUIRE_CALL(obj, getter(ANY(int)))
      .IN_SEQUENCE(s)
      .RETURN(0);

the C++11 version two API is,

    MAKE_EXPECTATION(obj, getter(ANY(int)),    
    e[0] = NAMED_REQUIRE_CALL(obj, getter(ANY(int)))
      .IN_SEQUENCE(s)
      .RETURN(0)
    );

This works because no additional scope is introduced by the MAKE_EXPECTATION macro. It cannot be written as

    e[0] = MAKE_EXPECTATION(obj, getter(ANY(int)),    
      NAMED_REQUIRE_CALL(obj, getter(ANY(int)))
        .IN_SEQUENCE(s)
        .RETURN(0)
      );

given the current implementation of the macro since the macro introduces a using statement into the token stream. Still, the current implementation can accomplish a lot of work.

This should work because the expansion of the REQUIRE_CALL11 macro will include the RETURN11() macro, with the same __LINE__, so they see the same ret_type.

This assumption turned out to be only partially correct. It holds for GCC and Visual Studio, but not for Clang.

This transcript shows that, for Clang, the value for __LINE__ varies at different points of the macro expansion.

$ g++ --version
g++ (Ubuntu 7-20170407-0ubuntu2) 7.0.1 20170407 (experimental) [trunk revision 246759]
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ g++ -E -x c++ - <<!
#define MAKE_EXPECTATION(...) \
  Is MAKE_EXPECTATIONs __LINE__ \
  __VA_ARGS__

#define RETURN(...) \
  = __LINE__ RETURNs \
  __VA_ARGS__

MAKE_EXPECTATION(
  RETURN(?)
)  
!

# 1 "<stdin>"
# 1 "<built-in>"
# 1 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 1 "<command-line>" 2
# 1 "<stdin>"

Is MAKE_EXPECTATIONs

 7
# 5 "<stdin>"
 = 7 RETURNs ?

# This is true for all versions of GCC we care about.

$ clang++-4.0 --version
clang version 4.0.0-1ubuntu1 (tags/RELEASE_400/rc1)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

$ clang++-4.0 -E -x c++ - <<!
#define MAKE_EXPECTATION(...) \
  Is MAKE_EXPECTATIONs __LINE__ \
  __VA_ARGS__

#define RETURN(...) \
  = __LINE__ RETURNs \
  __VA_ARGS__

MAKE_EXPECTATION(
  RETURN(?)
)  
!

# 1 "<stdin>"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 329 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "<stdin>" 2

Is MAKE_EXPECTATIONs 7 = 6 RETURNs ?

# This is true for all versions of Clang we care about.

As a result, I have not been able to remove the C++11 version one API.

AddressSanitizer issues with g++-7

There remains an issue with an AddressSanitizer error with g++-7 -std=c++14 stdc++. See this report on Travis for details,

https://travis-ci.org/AndrewPaxie/trompeloeil/jobs/264143994

The cause could be any of a number of issues:

rollbear commented 7 years ago

I'm having trouble putting this repetition of the signature to rest (feel free to ignore my ramblings if they disturb your progress.)

It occurred to me that a way around the __LINE__ hack may be to enclose it all in a lambda.

A macro like:

REQUIRE_CALL11(obj, func,...) \
  auto TROMPELOEIL_COUNT_ID(trompeloeil_expectation) = \
  [&](){ \
 using trompeloeil_calll_type = NAMED_MOCK_TYPE(obj1, count()); \
 return NAMED_REQUIRE_CALL(obj, func)
 ... /* ugly macro hackery here using trompeloeil_call_type */
}();

The idea being that the type is now local to the body of the lambda. Being in a local body, there's no need to use macro hackery to ensure a unique name for each invocation.

AndrewPaxie commented 7 years ago

I like this alternative and will see if it can be expanded into a a version three API. I have also been troubled by this repetition.

The g++-7 ASan error remains elusive. I am learning a great deal about ASan and the GCC implementation of regexes as I debug a small example with rr.

AndrewPaxie commented 7 years ago

Progress report

AddressSanitizer issues with g++-7

The issue with crashes running ASan with g++-7 was due to a defect in the compiler. This had been detected and reported by @MattGodbolt in June 2017. See

bugzilla.gcc.org, "Bug 81021 - stack-use-after-scope false positive error with exceptions," 8 June 2017. \ Available: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81021 \ Accessed: 23 August 2017

This has been marked as RESOLVED FIXED and an inspection of the source code for g++-7.2.0 released for Ubuntu Artful Aardvark shows that at least for this version of g++-7.2 the fix has been applied.

This doesn't mean that using ASan is going to be trouble-free. Consider, for example, this issue

"#837 - Recent kernel causes -fPIE ASan executables to abort on x86_64," \ Available: https://github.com/google/sanitizers/issues/837 \ Accessed: 23 August 2017

This means that currently on kernels such as that run by Artful Aardvark, a run with ASan enabled initializes about one time in ten and errors out the remainder of the times.

C++11 version three API

I have version three of the C++11 API implemented and committed, built upon your idea to use a lambda.

Implementation details

I managed to boil the implementation down to two macros, one for the case of zero variadic arguments and the other for the case of one or more variadic arguments. These are called, REQUIRE_CALL_0 and REQUIRE_CALL_V (for variadic, after similar macros in Boost.Preprocessor).

Here's the implementation of TROMPELOEIL_REQUIRE_CALL_V,

#define TROMPELOEIL_REQUIRE_CALL_V(obj, func, ...)                             \
  TROMPELOEIL_REQUIRE_CALL_V_(obj, func, #obj, #func, __VA_ARGS__)

#define TROMPELOEIL_REQUIRE_CALL_V_(obj, func, obj_s, func_s, ...)             \
  auto TROMPELOEIL_COUNT_ID(call_obj) =                                        \
    TROMPELOEIL_REQUIRE_CALL_V_LAMBDA(obj, func, obj_s, func_s, __VA_ARGS__)

#define TROMPELOEIL_REQUIRE_CALL_V_LAMBDA(obj, func, obj_s, func_s, ...)       \
  [&]                                                                          \
  {                                                                            \
    using s_t = decltype((obj).TROMPELOEIL_CONCAT(trompeloeil_self_, func));   \
    using e_t = decltype((obj).TROMPELOEIL_CONCAT(trompeloeil_tag_,func));     \
                                                                               \
    return TROMPELOEIL_REQUIRE_CALL_LAMBDA_OBJ(obj, func, obj_s, func_s)       \
      __VA_ARGS__                                                              \
      ;                                                                        \
  }()

#define TROMPELOEIL_REQUIRE_CALL_LAMBDA_OBJ(obj, func, obj_s, func_s)          \
  ::trompeloeil::call_validator_t<s_t>{(obj)} +                                \
      ::trompeloeil::detail::conditional_t<false,                              \
                                           decltype((obj).func),               \
                                           e_t>                                \
    {__FILE__, static_cast<unsigned long>(__LINE__), obj_s "." func_s}.func
API evolution
// C++11 expectation, version one
using m_t = NAMED_MOCK_TYPE(obj1, count());
REQUIRE_CALL(obj1, count())
  .IN_SEQUENCE(seq)
  .RETURN_TYPE(m_t, 1);

// C++11 expectation, version two
MAKE_EXPECTATION(obj1, count(),
REQUIRE_CALL(obj1, count())
  .IN_SEQUENCE(seq)
  .RETURN_TYPE(m_t, 1));

// C++11 expectation, version three
REQUIRE_CALL_V(obj1, count(),
  .IN_SEQUENCE(seq)
  .RETURN(1));

This is much more expressive and even closer to the C++14 API,

// C++14 expectation
REQUIRE_CALL(obj1, count())
  .IN_SEQUENCE(seq)
  .RETURN(1);

It also compiles and runs on all supported compiler versions.

I note in passing the _0 form for use when there are zero call modifiers,

REQUIRE_CALL_0(obj2, func());
Nothing comes for free

I have had to add -Wno-unneeded-member-function to the list of suppressed warnings in Clang, as it sees the functions trompeloeil_self_ ## name and trompeloeil_tag_ ## name as unneeded e.g. when the class declaration is embedded in an unnamed namespace. It would be good to add the [[maybe_unused]] attribute to their declarations when C++17 becomes a supported compiler mode.

C++11 version four API

I am looking for a way to collapse the REQUIRE_CALL_0 and REQUIRE_CALL_V macros into one REQUIRE_CALL_V macro. I took a look at how the function specifiers (override) were handled in the MAKE_MOCKn macros. I haven't been able to get rid of the trailing commas in the __VA_ARGS__. This wasn't an issue in MAKE_MOCKn as the __VA_ARGS__ was never referenced in the body of TROMPELOEIL_MAKE_MOCK_.

It is a shame that Standard C++ (and -Wpedantic) insists on there being at least one argument in __VA_ARGS__.

I will continue my search for a feasible solution while completing the next steps in getting this branch ready for a pull request. It is likely that if a version four C++11 API is indeed possible, I will deprecate and remove all prior versions of the C++11 API. This will considerably simplify what has to be reviewed and merged.

As always, any insights you have would be greatly appreciated.

rollbear commented 7 years ago

Really cool progress, I think.

__VA_ARGS__ is a pain for just the reason you say, and has been the source of many curses. One commonly used trick is to always add an extra parameter, ensuring that there's always one. But that just pushes the problem one step away and may not offer a solution anyway. In this case I think it would work, though. __VA_ARGS__ would then include the function, and you pass it on to another macro with REQUIRE_CALL_IMPL(obj, __VA_ARGS__, harmless_empty_modifier), where REQUIRE_CALL_IMPL is your current REQUIRE_CALL_V. It's just a matter of creating the harmless_empty_modifier (something that just returns *this, and give it a saner name.)

[[maybe_unused]] is of course easily handled:

#if __cplusplus >= 201703L
#  define TROMPELOEIL_MAYBE_UNUSED [[maybe_unused]]
#else
#  define TROMPELOEIL_MAYBE_UNUSED
#endif

I'm a bit confused about that, though. Do you mean you get this warning when compiling with the C++14 API? If not, I think it's better to ensure that the troubling members are never generated when compiling with the C++11 API.

AndrewPaxie commented 7 years ago

I had started in the direction you also suggest regarding __VA_ARGS__. I have called the empty modifier function null_modifier(). I will see if the "one more level of indirection" with the macro forwarding works in this case.

The -Wunneeded-member-function warning is generated with all versions of Clang, in both C++11 mode and C++14 mode. The context is

namespace
{
  class self_ref_mock
  {
  public:
    void expect_self()
    {
      exp = NAMED_REQUIRE_CALL_0(*this, mfunc());
    }
    MAKE_MOCK0(mfunc, void());
    std::unique_ptr<trompeloeil::expectation> exp;
  };
} /* unnamed namespace */

The warning occurs during the compilation of what MAKE_MOCK0(mfunc,void()) expands. Hope this helps.

As you suggest, I will continue to look for ways to avoid the warning rather than suppressing it.

AndrewPaxie commented 7 years ago

Progress report

C++11 API with a single macro for each expectation

The C++11 version three API consisted of two macros for each expectation,

Now with the help these two very enjoyable references,

Jens Gustedt, "Default arguments for C99," 3 June 2010. \ Available: https://gustedt.wordpress.com/2010/06/03/default-arguments-for-c99/ \ Accessed: 6 October 2017

Jens Gustedt, "Detect empty macro arguments," 8 June 2010. \ Available: https://gustedt.wordpress.com/2010/06/08/detect-empty-macro-arguments/ \ Accessed: 6 October 2017

your suggestions, and six weeks labour, I have indeed been able to collapse these two macros down to one, still named with the _V suffix. Indeed, the effect is to make the _0 macros optional.

It would have been easier with __VA_OPT__(),

Thomas Köppe, "Comma omission and comma deletion," ISO/IEC JTC1 SC22 WG21 P0306R4, 12 July 2017. \ Available: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0306r4.html \ Accessed: 1 October 2017

but that's why the language evolves.

Migration forward to C++14 is maintained as the macros remain disjoint and both sets are defined when in C++14 mode.

Implementation details

This is the code for TROMPELOEIL_REQUIRE_CALL_V(),

#define TROMPELOEIL_ADD_NULL_MODIFIER(...)                                     \
  TROMPELOEIL_ADD_NULL_MODIFIER_IMPL(__VA_ARGS__ .null_modifier())

#define TROMPELOEIL_ADD_NULL_MODIFIER_IMPL(...) __VA_ARGS__

#define TROMPELOEIL_REQUIRE_CALL_V(obj, func, ...)                             \
  TROMPELOEIL_REQUIRE_CALL_V_(obj,                                             \
                              func,                                            \
                              # obj,                                           \
                              # func,                                          \
                              TROMPELOEIL_ADD_NULL_MODIFIER(__VA_ARGS__))

This is the code for TROMPELOEIL_REQUIRE_CALL_0(),

// Optional.  For backward compatibility with earlier version three API.
#define TROMPELOEIL_REQUIRE_CALL_0(obj, func) \
  TROMPELOEIL_REQUIRE_CALL_V_(obj, func, # obj, # func, .null_modifier())

The remaining macros in the call chain,

TROMPELOEIL_REQUIRE_CALL_V_(obj, func, obj_s, func_s, ...)
TROMPELOEIL_REQUIRE_CALL_V_LAMBDA(obj, func, obj_s, func_s, ...)
TROMPELOEIL_REQUIRE_CALL_LAMBDA_OBJ(obj, func, obj_s, func_s)

are the same as in the previous version three API.

Next steps

You will see code that might better come from a third-party library like Abseil. I have tried to keep close to the intent of minimizing dependencies, and licensing issues are also avoided.

rollbear commented 7 years ago

Slightly off topic, but I'm presenting Trompeloeil again at the NDC{Techtown} conference on Friday next week, and I want to acknowledge your fabulous work.

What should I say about the state of C++11 support? I get the feeling that you're more or less finished, or am I reading too much into your update?

AndrewPaxie commented 7 years ago

That's good news. Say:

It's coming and we're working hard to have a high quality release by the end of October 2017.

I am cleaning up the obsolete API now. All known instances of the _0 macros were removed from my production code base yesterday. My hope is to have at least the code ready by end of this week and work on the documentation in weekend. Minor pull requests are in a known TODO list and will come prior to the c++11 branch pull request.

The limitations outlined in earlier progress reports remain:

These will be explained further in the documentation.

I will explain the journey (the what and the how) to the current best C++11 API in a series of blog posts once I have the current work list completed.

BTW I am attending the Pacific++ conference the week following your presentation, another motivation to achieve what I have set out to do.

AndrewPaxie commented 7 years ago

Progress Report

A quick note to let you know I was mauled by the preprocessor when trying to remove _0 macros. Somehow I managed to develop the proposed code in an environment where the -Wpedantic warning was not enabled. The implementation just isn't standard C++11/14.

Nevertheless, I have a backup set of macros, more complex, but working for g++ and clang++. Unfortunately the VC++ compiler has other ideas as to how to expand __VA_ARGS__, which I suspect you have encountered before (witness TROMPELOEIL_IDENTITY).

This will delay a few minor pull requests, but I still have wind in my sails and am tacking my way toward the finish line.

rollbear commented 7 years ago

Yeah, the preprocessor is the gift that keeps giving. MSVCs long standing bugs makes it even worse.

No worries. It's not like you have committed to a due date (and I won't give any at my talk on Friday, just say that "it's nearly done, but not quite yet.")

AndrewPaxie commented 7 years ago

I have completed a working implementation for MSVC and am running the regression tests on Travis now, https://travis-ci.org/AndrewPaxie/trompeloeil/builds/289857656 OSX builds running slow again as I write this, but I hope it will complete without errors.

Will refactor into a simpler form once other tasks are out of the way.

AndrewPaxie commented 7 years ago

Progress Report

I have more or less completed the remaining tasks before creating a pull request for your consideration.

Outstanding

During this review period, I rewrote all the support for C++14 standard library features missing in the C++11 standard library. These entities are now implemented solely using the draft proposals made to the C++ standards committee. This changes the attribution to these proposals instead of e.g. cppreference.com and puts the provenance of the code on firmer ground.

I have had one more attempt to fix the return type mismatch detection in the C++11 implementation of TROMPELOEIL_RETURN_(). Progress hasn't been enough to delay the C++11 pull request any longer. Any further work will have to be handled as a separate issue.

I look forward to crossing the finish line in the near future.

AndrewPaxie commented 7 years ago

Progress Report

Pull request #67 has been created for your first pass review and feedback.