All tests now pass under latest MSVC #47

frboyer · 2018-09-09T16:53:39Z

Hi,

I know you were still not trying to support MSVC, because of a weird bug, but that bug only affected one line which was easy to modify (yes, I know it's weird to have to change the name of a parameter). So here is a version compiling without warning at /W4 (except one in catch.hpp which is not part of this project) with latest MSVC, and passing all tests in both release and debug builds.

I added CMake files so it's easy to build with different systems; I compiled with make/g++, ninja/msvc and Visual Studio as tests.

Note that I also corrected some bugs at the same time, as MSVC was stricter on some undefined behavior that was used (comparing iterators on different containers and decrementing begin).

I hope you will find this interesting and merge it in the official branch.

- Currently on MSVC cl 19.15.26726, only 5/34 test files compile and pass tests

…alue (but kept tests using old syntax). - Fixed bug that operators * and -> should be const in IteratorIterator as they do not modify the iterator. - Changed name of template parameter from Container to T in iterator_type, because of a bug in MSVC. - Used an explicit decltype, instead of decltype(auto), in TupleStarMapper::IteratorData::get_and_call_with_tuple because MSVC was not able to deduce it. - Fixed bug in filterfalse_examples where <string> was not included. Result with MSVC: - All tests pass and examples are running, in release build. - 4/34 test files fail in debug, because of STL assertion failure.

…rbegin()-1 was used but is undefined behavior (see C++17 [bidirectional.iterators] and [random.access.iterators]). - Fixed incorrect test in test_sorted that was not checking the result of the comparision, but is disabled for now as it compares iterators on different containers, which is undefined behavior (see C++17 [forward.iterators]¶2). - Now all tests are passing with MSVC.

…ere it would slightly change tests) - Replaced std::all_of and any_of by fold expressions

…es the types used in the tests)

ryanhaining · 2018-09-11T23:10:32Z

There's a lot here that I can easily accept, some other things I need to take a closer look at to see if there's any cases that they might break. I'm gonna need some time here but I appreciate your effort, thank you

ryanhaining · 2018-09-28T23:27:25Z

internal/iterator_wrapper.hpp

@@ -32,15 +32,15 @@ namespace iter {

    template <typename Container>
    using IteratorWrapper = typename IteratorWrapperImplType<Container,
-        std::is_same<impl::iterator_type<Container>,
-            impl::iterator_end_type<Container>>{}>::type;
+        std::is_same_v<impl::iterator_type<Container>,


is this actually required for msvc?

Yes, it is. I should report the bug in msvc. It gives an "error C2512: 'std::is_same<Ts,Ts>': no appropriate default constructor available". There is a problem with the constexpr contructor when used in a variadic template. But anyway, since C++17, I would say that _v template variables are the preferred way, thus this modification follows C++17.

ryanhaining · 2018-09-28T23:28:42Z

internal/iteratoriterator.hpp

@@ -80,11 +80,11 @@ namespace iter {
        return ret;
      }

-      auto operator*() -> decltype(**sub_iter) {
+      auto operator*() const -> decltype(**sub_iter) {


This places a greater requirement on the sub_iter to have a const operator*()

If sub_iter is supposed to be an InputIterator, then it must have a const version of operator*. In C++17 standard:
[iterator.requirements.general]¶13 "In the following sections, a and b denote values of type X or const X"; then Table 95 "Input iterator requirements [...]" contains expression "* a".
Thus operator* must work on a const X.

If you think IteratorIterator should support also iterators that do not obey to the InputIterator requirements, both a const and non const version of the operators could be defined.

The library has always supported things as long as they have some operator*, so I think to keep that the same the way would be to add a const overload instead of replacing the nonconst. Do that and I'll merge

While trying to add a test case for non-const operator* (i.e. we should not add code unless there is a test that requires that code), I realized that IteratorIterator requires that TopIter is a RandomAccessIterator (requires the random_access_iterator_tag; there is a static_assert for that). Thus unless a class lies about being such an iterator, it must support const operator*. (Note that IteratorIterator also requires a const operator[], which I forgot in that version; I will correct that.)

Simple duplication of current operators *, -> and [], with const and non-const versions, would support an iterator having both the const and non-const versions (and also a standard iterator having only the const version). To do the tests requires to define a new special iterator, as vector::iterator does not have non-const versions.

To support non standard conforming iterators, without the const version, would require to remove the decltype return type of the operators, or use SFINAE, on the const version in IteratorIterator. Is this what you would like?

ah you're right I forgot about that requirement. In that case I think we might be fine as is with just const versions

Ok, so I added tests to verify that IteratorIterator operators are all working as specified by RandomAccessIterator in the standard, for both const and non-const iterators (having only const versions of *, -> and []). I passed each line of the requirements tables, I hope I did not miss anything (but I only test method results, I do not test the types).

I corrected the operator[] to be also const (as mentioned in my previous comment), to pass the RandomAccessIterator tests.

Those tests showed me that a weird operator- was not covered; in fact it was an incorrect operator where an expression like (2 - it) (subtracting an iterator from an integer) would give (it - 2). I thus simply removed that method.

ryanhaining · 2018-09-28T23:29:06Z

internal/iterbase.hpp

@@ -51,8 +51,8 @@ namespace iter {
    using AsConst = decltype(std::as_const(std::declval<T&>()));

    // iterator_type<C> is the type of C's iterator
-    template <typename Container>
-    using iterator_type = decltype(get_begin(std::declval<Container&>()));
+    template <typename T> //TODO: See bug https://developercommunity.visualstudio.com/content/problem/252157/sfinae-error-depends-on-name-of-template-parameter.html  for why we use T instead of Container.  Should be changed back to Container when that bug is fixed in MSVC.


given that this is fixed in 16.0 I'm not sure how long this is worth keeping around

I think the same. But that modification was required to verify now that everything is working on msvc. I'm not sure when 16.0 will be released; current preview is 15.9 pre 2.

…ssIterator - Removed incorrect operaror - where (2 - it) would give (it - 2) - Added tests to cover all methods in iteratoriterator.hpp

frboyer added 5 commits September 8, 2018 22:37

Added CMake files for better cross-platform build support.

08a40b5

- Currently on MSVC cl 19.15.26726, only 5/34 test files compile and pass tests

- Corrected all warnings at /W4 with MSVC (except in test_starmap, wh…

32f4dc2

…ere it would slightly change tests) - Replaced std::all_of and any_of by fold expressions

- Corrected warnings at /W4 with MSVC in test_starmap (slightly chang…

419a22a

…es the types used in the tests)

frboyer mentioned this pull request Sep 10, 2018

Add compiler compatibility list #25

Open

ryanhaining reviewed Sep 28, 2018

View reviewed changes

- IteratorIterator operator[] is now const, as required by RandomAcce…

54d6d5a

…ssIterator - Removed incorrect operaror - where (2 - it) would give (it - 2) - Added tests to cover all methods in iteratoriterator.hpp

ryanhaining merged commit c11673f into ryanhaining:master Oct 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All tests now pass under latest MSVC #47

All tests now pass under latest MSVC #47

frboyer commented Sep 9, 2018

ryanhaining commented Sep 11, 2018

ryanhaining Sep 28, 2018

frboyer Sep 29, 2018

ryanhaining Sep 28, 2018

frboyer Sep 29, 2018 •

edited

Loading

ryanhaining Sep 30, 2018

frboyer Sep 30, 2018

ryanhaining Sep 30, 2018

frboyer Oct 1, 2018

ryanhaining Sep 28, 2018

frboyer Sep 29, 2018

All tests now pass under latest MSVC #47

All tests now pass under latest MSVC #47

Conversation

frboyer commented Sep 9, 2018

ryanhaining commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frboyer Sep 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frboyer Sep 29, 2018 •

edited

Loading