Minor code cleanup questions

[MeanSquaredError]

While reading the new PR that adds NULLS FIRST/NULLS LAST, I came across a couple of places in the code that looked a slightly suspicious, so I decided to open a new issue about them:

1. Comments about CRTP. There are several structures that have comments in them that they should be used in CRTP:

• sqlpp23/include/sqlpp23/core/aggregate_function/enable_over.h

  Line 33 in f3f344d

  // To be used as CRTP base for aggregate expressions that should offer the

• sqlpp23/include/sqlpp23/core/operator/enable_comparison.h

  Line 33 in f3f344d

  // To be used as CRTP base for expressions that should offer the comparison

• sqlpp23/include/sqlpp23/core/operator/enable_as.h

  Line 35 in f3f344d

  // To be used as CRTP base for expressions that should offer the as() member

  
  All these comments about CRTP seem to be obsolete and probably should be updated. None of these structures are actually used with the CRTP pattern. I guess CRTP has been replaced by deducing this?

2. There are a multiple places where various structures inherit from enable_comparison which means that they all get the asc() and desc() methods. E.g. type cast expressions, case expressions and string concat expressions inherit from enable_comparison. I haven't actually tried with an actual test program, but given that they all inherit from enable_comparison, it seems to me that they all get the asc() and desc() methods. Aren't there too many expressions that get these methods? E.g. all of the ones that I listed above shouldn't get asc() and desc().

[rbock]

CRTP:
It is still CRTP, IMO. deducing this just makes it nicer.

enable_comparison:
You sort by CONCAT(t.first, t.last), for instance, can't you?

[MeanSquaredError]

CRTP: It is still CRTP, IMO. deducing this just makes it nicer.

IMHO, the CRTP pattern exists when the base class template receives the name of the derived class as a template argument, like this:

class derived : public base<derived>
{
    ...
};

So this would mean that the "deducing this" replaces the CRTP pattern, instead of modifying it. Actually one of the examples of P0847 (the C++ proposal for "deducing this"), talks about CRTP and says:

5.2 CRTP, without the C, R, or even T

Today, a common design pattern is the Curiously Recurring Template Pattern. This implies passing the derived type as a template parameter to a base class template as a way of achieving static polymorphism. If we wanted to simply outsource implementing postfix incrementation to a base, we could use CRTP for that. But with explicit objects that already deduce to the derived objects, we don’t need any curious recurrence — we can use standard inheritance and let deduction do its thing. The base class doesn’t even need to be a template:

This is link to the corresponding paragraph of P0847. So my guess is that the authors of "deducing this" also saw "deducing this" as a way to replace CRTP.

But I don't really insist on modifying the comments about CRTP. After all it is mostly a matter of terminology and one could still consider the current code a modified version of CRPT.

enable_comparison: You sort by CONCAT(t.first, t.last), for instance, can't you?

Yeah, you are correct. What I was was concerned about, was that expressions outside of the order_by clause would allow calling the asc()/desc() methods, like this:

select (min(tab.id).asc())

or maybe this

select (min(tab.id).asc().as(sqlpp::alias::a))

But after trying the above examples in actual test programs and thinking about it, I reached the conclusion that my concerns were unfounded. Calling asc()/desc() on an expression changes its type and the resulting expression cannot be used outside of order_by(). In the above examples, the first one won't compile because min(tab.id).asc() will not satisfy the select's requires clause. The second example will fail to compile because the return type of asc() does not have the method as(...). I guess similar problems will arise if one tries to use asc()/desc() outside of order_by(). So it should be OK the way it is now.