Cpp20 concepts : base module

[BDoignies]

PR Description

C++20 new concepts for the base module.

Checklist

• Unit-test of your feature with Catch.
• Doxygen documentation of the code completed (classes, methods, types, members...)
• Documentation module page added or updated.
• New entry in the ChangeLog.md added.
• No warning raised in Debug mode.
• All continuous integration tests pass (Github Actions)

[BDoignies]

Note: the documentation is not updated yet !

[BDoignies]

Hi @dcoeurjo,

Currently all concepts defined by DGtal have been rewritten with C++20 concepts (see https://github.com/BDoignies/DGtal/tree/Cpp20Concepts-missings, it was not PR though). What remains is the following:

• Clean up some BOOST_CONCEPT_ASSERT with simple boost concept (~= 180 lines left)
• Update all documentation to remove links to boost concept

HOWEVER

I would not recommend merging this PR chain, even though it is almost complete. It is kind of heartbreaking to admit that, because I have been working on this for quite some time. However, I think it is best for the library not to use it.
Below is the full rationale for why I dislike this PR, which I think is informative about how DGtal is compiled and may prove useful in the future.

I leave the PR open and it is up to you (@dcoeurjo) to decide whether to merge this PR (after the tweaks mentioned above) or leave it as an attempt that will not be merged.

TLDR:
DGtal relies so heavily on some "hidden mechanisms" (exploited by Boost), that this PR is a nightmare to use, to review and that would probably hurt both further development and usage. As it stands, boost_concept is cleaner to use.
As a whole, this PR gives me the impression of being very tricky, unstable and for no improvement.
other than "using the latest features", which is not a good reason for me.

Rationale :

Cpp code

I made the maccro we talked about, DGTAL_CONCEPT_CHECK, as versatile as possible. Its primary use was to reduce the burden of naming types, since we cannot use typedef and the like inside concepts. But this maccro turned out to be more useful than that, for reasons I personally do not like:

It changes WHEN the concept is checked

With or without the macro, the concept is checked at compile time. But in the case of DGtal it matters when during the compilation process. In short: C++20 concepts are checked when the type is "named". With the macro (and for that matter, with boost), concepts are checked when types and variables are instantiated. This is a bit of a nerdy difference, but let me show you some examples of why this is important:

• PointVector cannot be divided by unsigned integers. This is a strange point, since averages are everywhere, and dividing by their size is common. However, here is the actual definition of the operator/ that would be called (stripping away some templates):

    operator/ ( DGtal::PointVector<ptDim, LeftEuclideanRing, LeftContainer> const& lhs,
                RightScalar const& rhs )
        -> decltype( DGtal::constructFromArithmeticConversion(lhs, rhs) );

Unrolling the chain, the output type will be deduced from this class:

struct ArithmeticConversionTraits< PointVector<dim, LeftEuclideanRing, LeftContainer>, RightEuclideanRing,
      typename std::enable_if<
             IsArithmeticConversionValid<LeftEuclideanRing, RightEuclideanRing>::value
          && ! IsAPointVector<RightEuclideanRing>::value >::type >
  {
    using type = typename std::conditional<
      std::is_same< LeftEuclideanRing, ArithmeticConversionType<LeftEuclideanRing, RightEuclideanRing> >::value,
      PointVector<dim, LeftEuclideanRing, LeftContainer>,
      PointVector<dim, RightEuclideanRing> >::type;
  };

It is important to note that one of the possible results is: PointVector<dim, RightEuclideanRing>. However, if you divide by unsigned int, the actual type is PointVector<dim, unsigned int>... It cannot exist, because unsigned integers do not form a (Euclidean) ring. However, the current library works because this type is never actually used, as the condition is always in favour of PointVector<dim, LeftEuclideanRing> >::type, which is always valid in this context. When the requirements are done with simple C++20 concepts: PointVector cannot be divided by unsigned integers... However, with the macro, the concept is checked when the class is instantiated (ie. when the class is first used). As we never get such a PointVector<dim, unsigned int>, it never checks the concept and thus work.

The same reasoning applies to GMP (BigIntegers). Integers constructed with GMP (C++ version) sometimes do not fulfil the requirements of DefaultConstructible (which is one of the subconcepts of `CEuclideanRing').

This is also true for a few examples that use shortcuts with a 2D space, which is technically impossible because it builds a `ImplicitPolynomial3Shape' which requires the space to be 3D (BOOST_STATIC_ASSERT does not fail for the exact same reason).

• Undefined behaviour and incomplete types. A common pattern in the DGtal/image module (but it also appears elsewhere) is the following:

  template <typename I>
  class Toto
  {
  		typedef Toto<I> Self;
  		typedef SomeClassWithAConcept<Self> SomeName;
  };

Here the problem is different: Self, as far as the definition of SomeName is concerned, is an incomplete type. Most of the checks done with the std (constructible, assignable, ...) have the following warning (here from is_assignable):

	If T or U is not a complete type, (possibly cv-qualified) void, or an array of unknown bound, the behavior is undefined. 
	If an instantiation of a template above depends, directly or indirectly, on an incomplete type, and that instantiation could yield a different result if that type were hypothetically completed, the behavior is undefined. 

Again, using the macro in this context is important, as checking the concept "later" allows the type to be completed.

I caught these patterns because clang was nice enough not to compile this code. But as a result, I cannot guarantee that the PR does not introduce undefined behaviour.

• Board2D and Drawable concepts. As far as I know, this is an isolated example: Board2D::operator<< requires the concept CDrawableWithBoard2D, which requires the existence of Board2D. This is not allowed by C++20 concepts.
• Boost Concepts do not check what they should. For example: Forward Container are required to be comparable whenever the underlying type is. However, this is never checked by the boost concept (see: code)... DGtal relies on such things...
• The compilation time increased dramatically. I have not taken exact measurements, but this is noticeable when building all examples and tests. I think this is because concepts are now always checked, even if no type is used. This causes more work for the compiler...

MSVC...

• MSVC made design choices regarding concepts that caused some code to compile with gcc/clang and not with MSVC. Namely, it requires "string matching" for the concept, whereas g++/clang seems to require "normal form" matching. This means that, for example, the following gives a mismatching constraint error:

template <typename T> requires true class Toto { Toto(); };
template <typename T> requires (true) Toto<T>::Toto() {};

Note that the only differences between the concepts are the parentheses... As for namespaces, it is kind of random. Sometimes it can see through, sometimes not... This makes things very difficult for incoming developers... As a side note: this is intentional and compliant with the standard.

• MSVC crashes without saying why. Since I introduced some concept (I think it is CInteger) it "sometimes" crashes without any indication (internal compiler error). I've spent 3 days trying to find out why and haven't found a clue, although I thought I did. This is random and appears on classes that this PR does not even modify...

Reviewing this PR

• The PR is huge: in fact, it is so huge and redundant, it changes so many things, that I do not think it can be thoroughly reviewed. At least not in a reasonable time-frame.
• I am also sure that some concept are not strictly equivalent, either by design (this is documented when this happens), or by mistake.
• I am also sure that some concepts are not strictly equivalent, either by design (this is documented when this happens) or by mistake. I am almost certain that I have missed some checks that should only apply if the type is const, for example. Sorry :(
• If some parts of the library are not compiled by tests or examples, I cannot guarantee the code is valid (templates are only compiled when needed), though this should be minor. This is especially the case for Windows...
• This code of this PR is not written consistently.

Edit (1):

After writing this message, I noticed that some BOOST_CONCEPT_ASSERT have been commented for the reasons described above...