This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen

Bug 1619

Summary: const_iterator vs iterator compilation error
Product: Eigen Reporter: w.h.greene
Component: Core - generalAssignee: Nobody <eigen.nobody>
Status: RESOLVED FIXED    
Severity: Compilation Problem CC: brian.budge, chtz, gael.guennebaud, jacob.benoit.1
Priority: Normal Keywords: test-needed
Version: 3.4 (development)   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on:    
Bug Blocks: 814    
Attachments:
Description Flags
A patch for StlIterators and stl_iterators test none

Description w.h.greene 2018-11-01 10:28:20 UTC
I was experimenting with the new STL-compatible iterator capability using VS 2017. The following code snippet compiles correctly:

const int n = 6;
  typedef Eigen::VectorXd V;
  V v(n);
  v << 1, 4, 6, 17, 33, 8;
  cout << v.transpose() << endl;
  for (V::iterator it = v.begin(); it != v.end(); ++it)
    cout << *it << ", ";

However, if I replace V::iterator with V::const_iterator, VS produces these
compilation errors:

1>------ Build started: Project: eigenXTests, Configuration: Debug x64 ------
1>eigenXTests.cpp
1>c:\wgreene\tests\c++\vs2017tests\eigenxtests\eigenxtests.cpp(18): error C2440: 'initializing': cannot convert from 'Eigen::internal::pointer_based_stl_iterator<Derived>' to 'Eigen::internal::pointer_based_stl_iterator<const Derived>'
1>        with
1>        [
1>            Derived=Eigen::Matrix<double,-1,1,0,-1,1>
1>        ]
1>c:\wgreene\tests\c++\vs2017tests\eigenxtests\eigenxtests.cpp(18): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
1>c:\wgreene\tests\c++\vs2017tests\eigenxtests\eigenxtests.cpp(18): error C2679: binary '!=': no operator found which takes a right-hand operand of type 'Eigen::internal::pointer_based_stl_iterator<Derived>' (or there is no acceptable conversion)
1>        with
1>        [
1>            Derived=Eigen::Matrix<double,-1,1,0,-1,1>
1>        ]
1>c:\wgreene\src\eigen\eigen-3.3.x\eigen\src\core\stliterators.h(96): note: could be 'bool Eigen::internal::pointer_based_stl_iterator<const Derived>::operator !=(const Eigen::internal::pointer_based_stl_iterator<const Derived> &)'
1>        with
1>        [
1>            Derived=Eigen::Matrix<double,-1,1,0,-1,1>
1>        ]
1>c:\wgreene\tests\c++\vs2017tests\eigenxtests\eigenxtests.cpp(18): note: while trying to match the argument list '(Eigen::internal::pointer_based_stl_iterator<const Derived>, Eigen::internal::pointer_based_stl_iterator<Derived>)'
1>        with
1>        [
1>            Derived=Eigen::Matrix<double,-1,1,0,-1,1>
1>        ]
1>Done building project "eigenXTests.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========
Comment 1 Gael Guennebaud 2018-11-01 13:51:03 UTC
I can confirm with any compiler, the problem is that our iterator is not convertible to const_iterator. You can workaround using cbegin/cend.
Comment 2 Christoph Hertzberg 2018-11-01 14:01:38 UTC
I can also reproduce with gcc and clang.

The proper solution would be to allow conversion from non-const iterator to const_iterator.

Adding this is a partial solution:

   typedef pointer_based_stl_iterator<
           typename internal::remove_const<XprType>::type > non_const_iterator;
    template<typename XprType2>
    friend class pointer_based_stl_iterator;

    pointer_based_stl_iterator(const non_const_iterator& other)
            : m_ptr(other.m_ptr), m_incr(other.m_incr) {}

However, this does not cover the `operator==(iterator, const_iterator)`, etc comparisons (the other way around works directly).
In glibc++ iterator comparisons are templated allowing arbitrary `__normal_iterator` to be compared, if the underlying container matches.

And conversion has to be implemented for other types of iterators as well, like `Reshape<...>::iterator`.
Comment 3 Gael Guennebaud 2018-11-01 14:17:18 UTC
Sure, since I already wrote the unit tests I pushed them:

https://bitbucket.org/eigen/eigen/commits/2564b3d97bab242b5ad66bae5b5c914164cd05ff

I won't have time to work more on it for a few days.
Comment 4 Brian Budge 2018-11-01 21:30:32 UTC
I have a partial fix for this now: 

* I fixed the pointer_based_stl_iterator to enable const_iterator = iterator and other paired interactions.
* I fixed a couple of test cases that were backward.  Other than one of the random initializations that was previously failing, the rest of the stl_iterator test passes.

But I'm mulling over one piece that I'm not immediately sure how to solve:

* When the dimensionality isn't Nx1 or 1xN, iterator is still defined.  That means some checks (e.g. in googletest) are saying, "hey, I find that C::iterator is a thing".  This is because their test for usable C::iterator is based on SFINAE and they do something like this:

template <typename T>
bool IsContainer(int /*i*/, typename T::iterator * /*a*/ = NULL, typename T::iterator * /*b*/ = NULL) {
  return true;
}

template <typename T>
bool IsContainer(long /*l*/) {
  return false;
}

Then later because they detected there were iterators, they have code that uses those iterators.  Honestly their check could be better, but I don't they they expected void iterators :-).

Ideally, iterators would only be defined when they're usable, but I'm not coming up with any clever ways to make that happen.  Basically, I think DenseBase would need two separate definitions, one with iterators (Nx1, 1xN), and one without iterators (NxM).
Comment 5 Gael Guennebaud 2018-11-01 21:54:07 UTC
upgrading gtest should do the job, it is now implemented as:

typedef int IsContainer;
template <class C,
          class Iterator = decltype(::std::declval<const C&>().begin()),
          class = decltype(::std::declval<const C&>().end()),
          class = decltype(++::std::declval<Iterator&>()),
          class = decltype(*::std::declval<Iterator>()),
          class = typename C::const_iterator>
IsContainer IsContainerTest(int /* dummy */) {
  return 0;
}
Comment 6 Brian Budge 2018-11-01 22:13:25 UTC
Created attachment 890 [details]
A patch for StlIterators and stl_iterators test

I'm having a few issues getting with hg push in my fork, so just attaching a patch instead.
Comment 7 Brian Budge 2018-11-01 22:22:37 UTC
I guess given gtest's fix maybe it is fine for now to ignore the void iterator thing (I have some commented-out tests in my patch... please ignore those).  Thanks for pointing out gtest's fix.
Comment 8 Gael Guennebaud 2018-11-09 20:59:39 UTC
I applied a different patch with less boilerplate code:

For pointer-based:
https://bitbucket.org/eigen/eigen/commits/0810287d822f/

For indexed-based:
https://bitbucket.org/eigen/eigen/commits/f1fd9e95ac1c/
Comment 9 Nobody 2019-12-04 18:05:39 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/1619.