Issue 5258 - Implement Filter::As<>() method to make working with filters easier from C++ code

Issue 5258 - Implement Filter::As<>() method to make working with filters easier from C++ code

[sergei, 2017-10-12 12:24:34]

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Act...
File compiled/filter/ActiveFilter.h (right):

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Act...
compiled/filter/ActiveFilter.h:75: if ((mType & Type::ACTIVE) == Type::ACTIVE)
I wonder whether concrete type value can be a class member (enum, static const,
etc) and this method can be a template for all classes.

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Com...
File compiled/filter/CommentFilter.h (right):

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Com...
compiled/filter/CommentFilter.h:33: if ((mType & Type::COMMENT) ==
Type::COMMENT)
Tests would be very helpful, so far it seems it always returns nullptr when the
down-casting is actually possible and does cast when it's impossible.

[hub, 2017-10-12 13:51:33]

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Act...
File compiled/filter/ActiveFilter.h (right):

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Act...
compiled/filter/ActiveFilter.h:75: if ((mType & Type::ACTIVE) == Type::ACTIVE)
On 2017/10/12 12:24:34, sergei wrote:
> I wonder whether concrete type value can be a class member (enum, static
const,
> etc) and this method can be a template for all classes.

You can have static const, but then for linkage purpose it needs to be put in a
.cpp.

You can also have a class method like this

  static Type GetClassType()
  {
    return Type::FILTER;
  }

And then have Filter::As<> call T::GetClassType()

It will be statically looked up.

And since Filter doesn't have it calling As<Filter>() will fail.

[sergei, 2017-10-12 13:57:31]

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Act...
File compiled/filter/ActiveFilter.h (right):

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Act...
compiled/filter/ActiveFilter.h:75: if ((mType & Type::ACTIVE) == Type::ACTIVE)
On 2017/10/12 13:51:33, hub wrote:
> On 2017/10/12 12:24:34, sergei wrote:
> > I wonder whether concrete type value can be a class member (enum, static
> const,
> > etc) and this method can be a template for all classes.
> 
> You can have static const, but then for linkage purpose it needs to be put in
a
> .cpp.
> 
> You can also have a class method like this
> 
>   static Type GetClassType()
>   {
>     return Type::FILTER;
>   }
> 
> And then have Filter::As<> call T::GetClassType()
> 
> It will be statically looked up.
> 
> And since Filter doesn't have it calling As<Filter>() will fail.

A method is also fine for me, can we use constexpr? I would expect no overhead
from any of the approaches, even from static const which actually looks very
questionable but I have googled a bit regarding it while reviewing some another
review and it seems fine too.

[hub, 2017-10-12 14:03:01]

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Act...
File compiled/filter/ActiveFilter.h (right):

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Act...
compiled/filter/ActiveFilter.h:75: if ((mType & Type::ACTIVE) == Type::ACTIVE)

> A method is also fine for me, can we use constexpr? 

Yes you can. I recommend doing so.

[Wladimir Palant, 2017-10-13 11:43:29]

Making this consistent pulled in a large number of changes unfortunately - very
straightforward still.

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Act...
File compiled/filter/ActiveFilter.h (right):

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Act...
compiled/filter/ActiveFilter.h:75: if ((mType & Type::ACTIVE) == Type::ACTIVE)
On 2017/10/12 12:24:34, sergei wrote:
> I wonder whether concrete type value can be a class member (enum, static
const,
> etc) and this method can be a template for all classes.

Done.

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Com...
File compiled/filter/CommentFilter.h (right):

https://codereview.adblockplus.org/29574591/diff/29574592/compiled/filter/Com...
compiled/filter/CommentFilter.h:33: if ((mType & Type::COMMENT) ==
Type::COMMENT)
On 2017/10/12 12:24:34, sergei wrote:
> Tests would be very helpful, so far it seems it always returns nullptr when
the
> down-casting is actually possible and does cast when it's impossible.

This is C++ internals, so it cannot be tested directly. But it will be tested
indirectly by the matcher tests.

Yes, the condition was reversed here. With the code using this not quite done
yet, I didn't have a chance to test this code in action.

[sergei, 2017-10-13 12:34:37]

LGTM, just in case, I have left a couple of thoughts.

https://codereview.adblockplus.org/29574591/diff/29575605/compiled/subscripti...
File compiled/subscription/Subscription.h (right):

https://codereview.adblockplus.org/29574591/diff/29575605/compiled/subscripti...
compiled/subscription/Subscription.h:103: typename
std::enable_if<std::is_base_of<Subscription, T>::value>::type* = nullptr>
static_cast does not allow down casting at a compile time in the case of absent
corresponding inheritance, so I would expect that one can remove this SFINAE.
Even if it's required, in addition it might be a bit shorter (C++14)
typename = std::enable_if_t<std::is_base_of<Subscription, T>::value>.

https://codereview.adblockplus.org/29574591/diff/29575605/compiled/subscripti...
compiled/subscription/Subscription.h:109: return static_cast<T*>(this);
In addition, since T::classType seems already a convention, the body of this
method could be in some another function abp_down_cast where one can put any
kind of SFINAE checks regarding the possibility of inheritance or correctness of
T::classType, etc.

[hub, 2017-10-13 14:47:29]

LGTM

[Wladimir Palant, 2017-10-13 16:58:18]

https://codereview.adblockplus.org/29574591/diff/29575605/compiled/subscripti...
File compiled/subscription/Subscription.h (right):

https://codereview.adblockplus.org/29574591/diff/29575605/compiled/subscripti...
compiled/subscription/Subscription.h:103: typename
std::enable_if<std::is_base_of<Subscription, T>::value>::type* = nullptr>
On 2017/10/13 12:34:36, sergei wrote:
> static_cast does not allow down casting at a compile time in the case of
absent
> corresponding inheritance, so I would expect that one can remove this SFINAE.

You are correct, removed.

https://codereview.adblockplus.org/29574591/diff/29575605/compiled/subscripti...
compiled/subscription/Subscription.h:109: return static_cast<T*>(this);
On 2017/10/13 12:34:36, sergei wrote:
> In addition, since T::classType seems already a convention, the body of this
> method could be in some another function abp_down_cast where one can put any
> kind of SFINAE checks regarding the possibility of inheritance or correctness
of
> T::classType, etc.

Note that we have different checks for Filter and Subscription subclasses, the
latter getting the simpler version. With Subscription not having an extended
hierarchy I didn't consider implementing mType as a bitmask justified.

[hub, 2017-10-13 18:12:02]

LGTM

[sergei, 2017-10-13 18:40:53]

LGTM