New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
Bug 1619 - const_iterator vs iterator compilation error
Summary: const_iterator vs iterator compilation error
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: 3.4 (development)
Hardware: All All
: Normal Compilation Problem
Assignee: Nobody
URL:
Whiteboard:
Keywords: test-needed
Depends on:
Blocks: 3.4
  Show dependency treegraph
 
Reported: 2018-11-01 10:28 UTC by w.h.greene
Modified: 2018-11-09 20:59 UTC (History)
4 users (show)



Attachments
A patch for StlIterators and stl_iterators test (10.35 KB, patch)
2018-11-01 22:13 UTC, Brian Budge
no flags Details | Diff

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/

Note You need to log in before you can comment on or make changes to this bug.