This bugzilla service is closed. All entries have been migrated to
Bug 833 - Simplify Documentation by typedef-ing complicated return-types
Summary: Simplify Documentation by typedef-ing complicated return-types
Alias: None
Product: Eigen
Classification: Unclassified
Component: Documentation (show other bugs)
Version: 3.2
Hardware: All All
: Normal Documentation
Assignee: Nobody
Keywords: JuniorJob
Depends on:
Blocks: 402 998
  Show dependency treegraph
Reported: 2014-06-26 14:04 UTC by Christoph Hertzberg
Modified: 2019-12-04 13:27 UTC (History)
5 users (show)

Patch for fixing return types in CwiseUnaryOps (5.91 KB, patch)
2014-07-03 18:20 UTC, Martin Beeger
no flags Details | Diff

Description Christoph Hertzberg 2014-06-26 14:04:35 UTC
Looking at return types like (from

internal::cast_return_type<Derived,const CwiseUnaryOp<internal::scalar_cast_op<typename internal::traits<Derived>::Scalar, NewType>, const Derived> >::type

is more likely to confuse the average user. We could hide these into \internal typedefs/helper structs like these:
  //! \internal Return type of cast()
  template<typename NewType>
  struct CastReturnType {
    typedef /* complicated stuff here */ type;

  //! cast function:
  template<typename NewType>
  CastReturnType<NewType>::type cast() const;

Actually, we already do that for many functions.

An additional advantage of this would be to simplify the storage of unevaluated expressions without needing the C++11 auto keyword.

Fixing this is mostly copy-and-paste and does not require deep understanding of Eigen's internals --> JuniorJob
Comment 1 Martin Beeger 2014-07-03 18:20:15 UTC
Created attachment 472 [details]
Patch for fixing return types in CwiseUnaryOps
Comment 2 Martin Beeger 2014-07-03 18:24:02 UTC
While creating this patch a few questions came up:
Why is in some cases the internal type trait for SCalar gives and sometypes the Scalar type? 
In one case even header and body differ in that regard.
Is there consistent naming pattern which is documented somewhere?
I tried to be as close as possible to whatever was already there.

Also this patch is still untested because I expect that some changes will be necessary as soon I got some answers to above questions.
Comment 3 Christoph Hertzberg 2014-07-03 19:00:45 UTC
Comment on attachment 472 [details]
Patch for fixing return types in CwiseUnaryOps

Review of attachment 472 [details]:

Very good starting point. Of course the same will be needed for the CwiseBinaryOps

::: Eigen/src/plugins/CommonCwiseUnaryOps.h
@@ +43,5 @@
>  typedef CwiseUnaryView<internal::scalar_imag_ref_op<Scalar>, Derived> NonConstImagReturnType;
> +/** \internal the return type of cast() */
> +typedef typename internal::cast_return_type<Derived,
> +				const CwiseUnaryOp<internal::scalar_cast_op<typename internal::traits<Derived>::Scalar, NewType>, const Derived>
> +				>::type CastReturnType;

This will not work, because CastReturnType depends on NewType. The only C++03 compatible way is an internal helper struct similar to FixedSegmentReturnType (and others) in "BlockMethods.h"
I'm not sure if the uppercase typedef (...) Type; is intended (maybe because it is, as it can be considered part of the interface), but I suggest to stay consistent with that.
Comment 4 Christoph Hertzberg 2014-07-03 19:10:58 UTC
(In reply to comment #2)
> Why is in some cases the internal type trait for SCalar gives and sometypes the
> Scalar type? 

In some cases it is not possible to use Scalar directly, because it is only defined inside the class. But if the implementation is inside the class, I don't think using Scalar directly should be a problem. Maybe in some cases things were moved from out-of-class definition to in-class definition, but the return type was not changed.

> In one case even header and body differ in that regard.

You can actually replace all occurrences inside the body by your new typedefs. This will even reduce code duplication.

> Is there consistent naming pattern which is documented somewhere?

I doubt anything regarding that is documented. I guess ${operation_as_noun}ReturnType is commonly used.
Comment 5 Christoph Hertzberg 2014-09-23 19:37:17 UTC
Partially, this has been fixed by this pull request:
There is still room for discussion, how to name the typedefs, and where and how they should be visible in the doxygen documentation.
Comment 6 Christoph Hertzberg 2014-09-29 13:50:14 UTC
I did some experiments with macros and Doxygen, it seems to be perfectly possible to avoid a lot of boiler-plate code by things like this (simplified example):

  /** \typename RETURN_TYPE The return type of NAME(). */   \
  typedef CwiseUnaryOp<internal::OPERATOR> RETURN_TYPE;     \
  /** \returns an expression of the coefficient-wise DOCUMENTATION of \c *this */   \
  inline RETURN_TYPE NAME() const {                         \
    return RETURN_TYPE(derived());                          \

Which could then be used like:

EIGEN_GENERATE_UNARY(Abs2, Abs2ReturnType, scalar_abs2_op, squared absolute value)

And Doxygen happily inserts macro parameters inside the comment. The only unfortunate problem is that commas are not usable inside macro parameters, at least not without further tricks. This will make even common things like
  \sa method1, method2, method3
harder than at the moment. However, it could be solved using an alias or a macro which generates a comma.
Comment 7 Gael Guennebaud 2014-09-30 17:43:19 UTC
We could pass (\sa method1, method2, method3) as a single argument, but then without variadic macro I don't see how to strip the parentheses without knowing the number of commas in advance.
Comment 8 Christoph Hertzberg 2014-09-30 19:09:50 UTC
Actually, doxygen appears to accept variadic macros. That means we could do something like this:

#define STRIP_PAREN(...) __VA_ARGS__


which can be used as:
  EIGEN_G_U(abs2, Abs2ReturnType, scalar_abs2_op,
     (here comes documentation, which can, include, commas,!) )

This should not break C++98/03 compatibility, since the actual compiler will never remove the parentheses around the DOCU parameter.

The only drawback I see with this macro approach is that it might confuse some IDEs. But I guess IDEs which are fine with our current #include "*plugin.h" inside class bodies should be able to handle this as well.
Comment 9 Gael Guennebaud 2014-10-01 22:23:01 UTC
Then we're good. I'm not afraid by IDE because handling code within macro is much common than #include within class bodies and our inheritance mechanism is quite tricky too (e.g., to find that MatrixBase is base class of Matrix).
Comment 10 Nobody 2019-12-04 13:27:34 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to'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:

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