-
Notifications
You must be signed in to change notification settings - Fork 118
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
All tests now pass under latest MSVC #47
Changes from 5 commits
08a40b5
39158fb
9d6bf0e
32f4dc2
419a22a
54d6d5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# Note that some examples currently use boost.optional which we do no not search for in this file. | ||
# You might have to use the "keep going" option to continue building on errors if boost.optional is not in the include path. | ||
# For example, building with MSVC (from an examples/buildMsvc directory): | ||
# set CXX=cl.exe | ||
# cmake .. -G Ninja | ||
# cmake --build . -- -k99 | ||
|
||
cmake_minimum_required(VERSION 3.8) | ||
project(cppitertools_examples CXX) | ||
set (CMAKE_CXX_STANDARD 17) | ||
|
||
include_directories( | ||
.. | ||
) | ||
|
||
file(GLOB _examples_files "*_examples.cpp") | ||
|
||
foreach(_file_cpp ${_examples_files}) | ||
get_filename_component(_name_cpp "${_file_cpp}" NAME) | ||
get_filename_component(_name_without_extension "${_name_cpp}" NAME_WE) | ||
add_executable(${_name_without_extension} ${_file_cpp}) | ||
endforeach() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,11 +80,11 @@ namespace iter { | |
return ret; | ||
} | ||
|
||
auto operator*() -> decltype(**sub_iter) { | ||
auto operator*() const -> decltype(**sub_iter) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This places a greater requirement on the sub_iter to have a const operator*() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If sub_iter is supposed to be an InputIterator, then it must have a const version of operator*. In C++17 standard: 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah you're right I forgot about that requirement. In that case I think we might be fine as is with just const versions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return **this->sub_iter; | ||
} | ||
|
||
auto operator-> () -> decltype(*sub_iter) { | ||
auto operator-> () const -> decltype(*sub_iter) { | ||
return *this->sub_iter; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. given that this is fixed in 16.0 I'm not sure how long this is worth keeping around There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
using iterator_type = decltype(get_begin(std::declval<T&>())); | ||
|
||
// iterator_type<C> is the type of C's iterator | ||
template <typename Container> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
# Note that some examples currently use boost.optional which we do no not search for in this file. | ||
# You might have to use the "keep going" option to continue building on errors if boost.optional is not in the include path. | ||
# For example, building with MSVC (from a test/buildMsvc directory): | ||
# set CXX=cl.exe | ||
# cmake .. -G Ninja | ||
# cmake --build . -- -k99 | ||
|
||
cmake_minimum_required(VERSION 3.8) | ||
project(cppitertools_tests CXX) | ||
set (CMAKE_CXX_STANDARD 17) | ||
|
||
include_directories( | ||
.. | ||
) | ||
|
||
include(CheckIncludeFileCXX) | ||
set(CMAKE_REQUIRED_INCLUDES ${PROJECT_SOURCE_DIR}) | ||
CHECK_INCLUDE_FILE_CXX(catch.hpp _has_catch) | ||
if(NOT "${_has_catch}") | ||
message("WARNING: catch.hpp not found, run ./download_catch.sh from test/ directory first") | ||
endif() | ||
|
||
file(GLOB test_sources RELATIVE ${PROJECT_SOURCE_DIR} "test_*.cpp") | ||
list(REMOVE_ITEM test_sources test_main.cpp) | ||
add_library(test_main OBJECT test_main.cpp) | ||
|
||
foreach(_source_cpp ${test_sources}) | ||
get_filename_component(_name_without_extension "${_source_cpp}" NAME_WE) | ||
add_executable(${_name_without_extension} ${_source_cpp} $<TARGET_OBJECTS:test_main>) | ||
endforeach() | ||
|
||
add_executable(test_all ${test_sources} $<TARGET_OBJECTS:test_main>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this actually required for msvc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.