This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen
Bug 124 - our constness helpers don't do the same as like-named STL helpers
Summary: our constness helpers don't do the same as like-named STL helpers
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: unspecified
Hardware: All All
: --- Unknown
Assignee: Hauke Heibel
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 3.0-beta3
  Show dependency treegraph
 
Reported: 2010-11-26 15:21 UTC by Benoit Jacob
Modified: 2019-12-04 10:02 UTC (History)
3 users (show)



Attachments
Implements the proposed changes. (2.34 KB, patch)
2010-11-26 16:59 UTC, Hauke Heibel
no flags Details | Diff
The complete patch. (10.71 KB, patch)
2010-11-26 17:10 UTC, Hauke Heibel
jacob.benoit.1: review-
Details | Diff

Description Benoit Jacob 2010-11-26 15:21:23 UTC
This follows up on bug 85.

In presence of pointer types T*, our constness helpers work on the value type T, whereas STL helpers don't special-case pointer types.

hence:

  us:   T*  -> const T*
  stl:  T*  -> T* const

This applies to remove_const and add_const.

I suggest to rename our helpers to slightly different names to prevent confusion with STL. How about:

   add_const   ->   add_const_on_value_type
   remove_const ->  remove_const_on_value_type

We should then have add_const and remove_const behave exactly like in the stl.
Comment 1 Benoit Jacob 2010-11-26 15:27:36 UTC
Another bug:

template<typename T> struct remove_const { typedef T type; };
template<typename T> struct remove_const<const T> { typedef T type; };
template<typename T> struct remove_const<T const &> { typedef T & type; };
template<typename T> struct remove_const<T const *> { typedef T * type; };

Here, if T = const U* const, since that's a constant type, the first specialization may be taken by the compiler, resulting in const U*, as the STL would do. But if T = const U*, we return U*, unlike the STL.
Comment 2 Benoit Jacob 2010-11-26 15:44:12 UTC
According to:

http://publib.boulder.ibm.com/infocenter/comphelp/v8v101/index.jsp?topic=%2Fcom.ibm.xlcpp8a.doc%2Flanguage%2Fref%2Fpartial_specialization.htm

"If the compiler finds more than one specialization, then the compiler tries to determine which of the specializations is the most specialized. A template X is more specialized than a template Y if every argument list that matches the one specified by X also matches the one specified by Y, but not the other way around. If the compiler cannot find the most specialized specialization, then the use of the class template is ambiguous; the compiler will not allow the program."

In our case (remove_const), that doesn't make a difference as only the specialization for const T matches const U* const so we're really buggy. But it means that (provided it's unit tested) it should still be OK to implement these helpers by specializing for N cases as we currently do.
Comment 3 Hauke Heibel 2010-11-26 16:59:57 UTC
Created attachment 40 [details]
Implements the proposed changes.

The attached patch should fix this issue. I tested via


#include <cassert>
#include <iostream>
#include <type_traits>

#include <Eigen/Core>

using namespace Eigen;

int main()
{
  typedef float U;

  static_assert(std::is_same< std::remove_const<U const*>::type, internal::remove_const<U const*>::type >::value, "remove_const");
  static_assert(std::is_same< std::remove_const<const U*>::type, internal::remove_const<const U*>::type >::value, "remove_const");
  static_assert(std::is_same< std::remove_const<const U*>::type, internal::remove_const<const U*>::type >::value, "remove_const");

  static_assert(std::is_same< std::add_const<U>::type, internal::add_const<U>::type >::value, "add_const");
  static_assert(std::is_same< std::add_const<U*>::type, internal::add_const<U*>::type >::value, "add_const");
  static_assert(std::is_same< std::add_const<U&>::type, internal::add_const<U&>::type >::value, "add_const");

  static_assert(std::is_same< internal::add_const_on_value_type<U&>::type, U const& >::value, "add_const_on_value_type");
  static_assert(std::is_same< internal::add_const_on_value_type<U*>::type, U const* >::value, "add_const_on_value_type");
 
  static_assert(std::is_same< internal::add_const_on_value_type<U>::type, const U >::value, "add_const_on_value_type");
  
  static_assert(std::is_same< internal::add_const_on_value_type<const U>::type, const U >::value, "add_const_on_value_type");
  static_assert(std::is_same< internal::add_const_on_value_type<U const>::type, const U >::value, "add_const_on_value_type");

  static_assert(std::is_same< internal::add_const_on_value_type<const U* const>::type, const U* const>::value, "add_const_on_value_type");
  static_assert(std::is_same< internal::add_const_on_value_type<U* const>::type, const U* const>::value, "add_const_on_value_type");
}
Comment 4 Hauke Heibel 2010-11-26 17:00:54 UTC
In fact, my local history also contains the renames.
Comment 5 Hauke Heibel 2010-11-26 17:10:36 UTC
Created attachment 41 [details]
The complete patch.
Comment 6 Benoit Jacob 2010-11-26 17:19:46 UTC
Comment on attachment 41 [details]
The complete patch.

-template<typename T> struct remove_const { typedef T type; };
-template<typename T> struct remove_const<const T> { typedef T type; };
-template<typename T> struct remove_const<T const &> { typedef T & type; };
-template<typename T> struct remove_const<T const *> { typedef T * type; };
+template<typename T> struct remove_const_on_value_type { typedef T type; };
+template<typename T> struct remove_const_on_value_type<const T> { typedef T type; };
+template<typename T> struct remove_const_on_value_type<T const &> { typedef T & type; };
+template<typename T> struct remove_const_on_value_type<T const *> { typedef T * type; };

This is still buggy, right?

remove_const_on_value_type<U const * const> will match the first specialization on <const T> hence will return T const *. It won't match at all the specialization on T const * because there is no type T making T const * be the same as U const * const.

So:
  - please expand the meta.cpp unit test
  - may I suggest always putting const after the type, it will be far less confusing in this context.
Comment 7 Hauke Heibel 2010-11-26 18:19:05 UTC
Just an additional note, so nobody gets confused. The errors from the patch
above are fixed. Actually, we have decided on IRC to remove
"remove_const_on_value_type" since

a) its semantics is confusing
b) more importantly - we don't use it.

The patch will be pushed one the tests have been run.
Comment 8 Nobody 2019-12-04 10:02:03 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/124.

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