Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm doing some high energy physics modelling in C++. I have written code that implements class that score interactions of particles with detector material.

Here is a base class:

class XYZScorer: public virtual AbstractScorer{
public:

    XYZScorer(double material_rad_len=-1):
        AbstractScorer(material_rad_len){;}

    virtual void RegisterHit(const XYZHit hit)=0;

};

Function RegisterHit registers single interaction between particle and detector. There are many scorers in this program.

Since modeling experimental data is quite hard, I need to be able to add some features to these scorers and I'll need to be able to easily switch these features on and off. I decided to implement these features using a decorator pattern. Here is decorator base class.

template <typename Scorer>
class ScorerDecorator: public virtual XYZScorer{

protected:
    Scorer scorer;
public:
    ScorerDecorator(Scorer scorer): XYZScorer(scorer.material_rad_len), scorer(scorer){;}
    virtual ~ScorerDecorator(){}

    virtual void SetEvent(size_t new_incident){
        scorer.SetEvent(new_incident);
    }

    virtual void CloseAfterRunEnd(){
        scorer.CloseAfterRunEnd();
    }
};

Here are two implementations of these decorators:

template <typename Scorer>
class FixZToFirstInteractionScorer: public virtual ScorerDecorator<Scorer>{
private:
    bool fixed_in_this_event=false;
    double position_of_first_intraction=0;
public:

    FixZToFirstInteractionScorer(Scorer scorer): ScorerDecorator<Scorer>(scorer){}
    virtual ~FixZToFirstInteractionScorer(){}

    virtual void SetEvent(size_t new_incident){
        fixed_in_this_event=false;
        ScorerDecorator<Scorer>::SetEvent(new_incident);
    }

    virtual void RegisterHit(const XYZHit hit){
        if(!fixed_in_this_event){
            fixed_in_this_event=true;
            position_of_first_intraction=hit.z;
        }
        XYZHit fixed_coords = hit;
        fixed_coords.z-=position_of_first_intraction;
        this->scorer.RegisterHit(fixed_coords);
    }

};

And another one:

template<typename Scorer>
class ApplyEnergyCutsScorer: public virtual ScorerDecorator<Scorer>{
    const double e_cut_energy;
public:
    ApplyEnergyCutsScorer(Scorer scorer):
        ScorerDecorator<Scorer>(scorer), e_cut_energy(GodObject::GetEcut()){}
    virtual ~ApplyEnergyCutsScorer(){}
inline bool accept(const XYZHit hit){
    double kin_energy = hit.GetParticleKineticEnergy();
    if(e_cut_energy < 0){
        return true;
    }
    if (kin_energy <0){
        throw new invariant_error("Energy less than zero --- normally it signifies that step wasnt passed to XYZHit");
    }
    if (kin_energy < e_cut_energy){
        return false;
    }
    return true;
}

virtual void RegisterHit(const XYZHit hit){
    if (accept(hit)){
        this->scorer.RegisterHit(hit);
    }
}

};

So far so good:

  • Note that due to template usage and scorer not being a pointer there is no virtual function call here at all. This quite important for me because: RegisterHit function is called for every interaction of any particle with the detector (which is quite often), and having multiple virtual table lookups could be slow (I guess).
  • I can wrap a class in multiple decorators, and compiler will unwrap and inline everyting (I checked that).

My problem is with instantiating these decorators, wrapping three of them in single method is ugly. Anyways here is the example (from production code):

template<typename Scorer>
std::shared_ptr<XYZScorer> InitializeAnyScorer(
        std::shared_ptr<AbstractGeometry> geometry, Scorer scorer, double cut_off_energy){
    if (!geometry->IsEnabled()){
        return std::shared_ptr<XYZScorer>(0);
    }

    std::shared_ptr<XYZScorer> result_scorer;
    if (!geometry->GetMoveZToFirstInteraction()){
        result_scorer = std::shared_ptr<ApplyEnergyCutsScorer<Scorer>>(
                 new ApplyEnergyCutsScorer<Scorer>(
                         scorer
                 )
             );
    }else{
        result_scorer = std::shared_ptr<FixZToFirstInteractionScorer<ApplyEnergyCutsScorer<Scorer>>>(
                new FixZToFirstInteractionScorer<ApplyEnergyCutsScorer<Scorer>>(
                    ApplyEnergyCutsScorer<Scorer>(
                            scorer
                )
            )
        );
    }

    return result_scorer;
}

How can I deal with ugliness of instantiating of my decorated scorers?

  • Is there any better decorator pattern in C++ that is performed compile time (I want all decorated functions to be inlineable), and applicable to my problem?
  • How can my decorated scorers be instantiate in a nicer way? I was considering a variadict template function but I surrendered.

Ideally I'd like to be able to use something similar to:

std::shared_ptr<XYZScorer> ptr 
     = decorate(scorer, FixZToFirstInteractionScorer, ApplyEnergyCutsScorer); 

Non issues

  • Both my code and any solution can work in a single thread mode.

Solution

I'd like to thank both @Corbin and @iavr, both answers were obviously right, and also very informative. I ended up using solution 3 from @iavr, which is very usable and not so magical :).

Here is example code:

template<typename S>
std::shared_ptr<decorete_decay<S>> decorate_scorer(S&& scorer)
{
    return std::make_shared<decorete_decay<S>>(std::forward<S>(scorer));
}


BOOST_AUTO_TEST_CASE(TestCompositeScorer2){

    auto test = decorate_scorer(FixZToFirstInteractionScorer<ApplyEnergyCutsScorer<TestScorer>>(TestScorer()));
    test->RegisterHit(XYZHit(1, 2, 3, 4, CreateStep(9)));
    /* tests */ 
}
share|improve this question

2 Answers 2

up vote 10 down vote accepted

I might have a number of minor stylistic comments but this is not important. What is important is the desired function decorate():

template <typename S>
using result = typename std::result_of <S>::type;

template <template <typename> class E, template <typename> class... D>
struct decorate_impl
{
    template <typename A, typename N = result<decorate_impl<D...>(A)>>
    E<N> operator()(A&& a) const
    {
        return E<N>(decorate_impl<D...>()(std::forward<A>(a)));
    }
};

template <template <typename> class E>
struct decorate_impl <E>
{
    template <typename A, typename N = typename std::decay<A>::type>
    E<N> operator()(A&& a) const { return E<N>(std::forward<A>(a)); }
};

template <template <typename> class E, template <typename> class... D, typename A>
std::shared_ptr <E<result<decorate_impl<D...>(A)>>)
decorate(A&& a)
{
    return std::make_shared <E<result<decorate_impl<D...>(A)>>>
        (decorate_impl<D...>()(std::forward<A>(a)));
}

which can be used like

std::shared_ptr<XYZScorer> ptr =
    decorate<FixZToFirstInteractionScorer, ApplyEnergyCutsScorer>(scorer);

This is very close to the desired syntax (maybe better), generic with respect to number and type of arguments, and inlinable.

Here is a live example, where I've thrown some arbitrary code to fill in the missing pieces of your code so that the whole thing compiles.

Attempt 2


I have now realized that the entire approach is wrong. We have a number of class templates and we want to apply them to a given object sequentially, constructing a nested object. To do this, we have a list of the class templates and we ask a single function decorate() to do the entire job for us, using recursion.

This is not right. The class templates themselves should not be used directly. We should have a helper function to wrap a given object into each class individually, exactly what std::make_shared() does for std::shared_ptr (similarly make_tuple, make_pair etc.). So here it is:

template <typename S>
using decay = typename std::decay<S>::type;

template <typename S>
FixZToFirstInteractionScorer<decay<S>> fixZToFirstInteraction(S&& scorer)
{
    return FixZToFirstInteractionScorer<decay<S>>(std::forward<S>(scorer));
}

template <typename S>
ApplyEnergyCutsScorer<decay<S>> applyEnergyCuts(S&& scorer)
{
    return ApplyEnergyCutsScorer<decay<S>>(std::forward<S>(scorer));
}

template<typename S>
std::shared_ptr<decay<S>> share(S&& scorer)
{
    return std::make_shared<decay<S>>(std::forward<S>(scorer));
}

to be used like

std::shared_ptr<XYZScorer> ptr =
    share(fixZToFirstInteraction(applyEnergyCuts(scorer)));

See new live example, where I have both approaches and a validation that they produce the same result. In fact, I've discovered a mistake in my first attempt, which I have now corrected.

I think this is cleaner and probably easier to use. Its implementation is easier to understand (it's not so "magical" as the first attempt) and certainly easier for the compiler (so, faster to compile). Each object is created at the right time when its type is known, and no recursion is needed to find all unknown return types of deeply nested function calls as with decorate(). Maybe I was carried away by the "desired syntax" and didn't see this immediately.

Of course, this means we have to write one more function for each new scorer class template. But all these functions are so small and similar. If we are very lazy we could even write a macro for them.

Note that in this case we are also constructing the outermost Scorer before making the shared_ptr, which we didn't in the first attempt. The reason is that otherwise we would need two functions for each class template, one returning an object on the stack and another a shared_ptr. In case Scorer is a large object, a move constructor will ensure that we don't loose performance with the unnecessary object.

Attempt 3


Never giving up. New live example.

Without decorate(), without any helper functions (except share()), just this:

std::shared_ptr<XYZScorer> ptr3 =
    share(FixZToFirstInteractionScorer<ApplyEnergyCutsScorer<Scorer>>(scorer));

It just works! Why? FixZToFirstInteractionScorer<ApplyEnergyCutsScorer<Scorer>> expects an ApplyEnergyCutsScorer<Scorer> argument and we give it just a Scorer. But an ApplyEnergyCutsScorer<Scorer> is implicitly constructed from the Scorer. All this happens automatically without touching the code, and will work under arbitrary nesting.

We could even write a new decorate() around this idea, which would be purely metaprogramming now. I may do this, or not.

share|improve this answer
    
Thanks! Would you care to elaborate how it works? Or is it some kind of patter I can read about (if so where). –  jb. Apr 13 at 22:01
1  
Wow. That is some impressive template magic! Wish I could more than +1 this. –  Corbin Apr 13 at 22:03
1  
@jb Also note I've edited the code to make it much simpler than in my original post, and I now use std::make_shared as suggested by Corbin (thanks). –  iavr Apr 13 at 22:21
1  
@jb I added another solution which is much simpler, and I think this is the correct one. If you are happy with this, then maybe I am spared from explaining the previous "magical" one ;-) –  iavr Apr 14 at 0:16
1  
@jb And a third solution... –  iavr Apr 14 at 0:54

I need a bit to look into a cleaner decorator pattern, but three technicalities immediately jump out at me.


Accepting a user defined value as const implies that your methods should probably be taking const-references. void f(const T) will make a copy of the value passed to it. void f(const T&) will not, yet it maintains the restriction that T cannot be mutated. Unless there's something I'm missing, I can't see why you're taking all of the XYZHit by const value.

The rules can be a bit more complicated, but in most cases:

  • If you want to mutate an object and have the changes visible from the outside, accept by reference.
  • If you want to mutate an object and not have the changes visible from the outside, accept by value (allowing you to change the copy, not the passed value).
  • If you do not want to mutate the object, accept by const-reference.

Another issue I immediately see is that you're instantiating shared_ptr directly whereas you should always use make_shared.

There are certain exception safety issues with shared_ptr in the general case (for example: what if you have shared_ptr(new T, new R) and the new T succeeds then new R throws? R will leak).

More importantly for your case though, make_shared exhibits better performance since it can roll the reference count (which has to be heap allocated) and the object allocation into the same allocation. Not only does this save an allocation, but it can also improve cache coherency depending on usage patterns.

As a bonus, it's also a tiny bit less repetitive (though not meaningfully -- it just removes one duplicated type). For example, your biggest instantiation becomes:

std::make_shared<FixZToFirstInteractionScorer<ApplyEnergyCutsScorer<Scorer>>>(ApplyEnergyCutsScorer(scorer));

When you're not taking (partial) ownership of a shared_ptr, you should take it by const-reference. This allows you avoid making an unnecessary copy of the pointer which will in turn do significant bookkeeping work behind the scenes only to undo it when the function returns. Since InitializeAnyScorer has no interest in becoming a partial owner of geometry, there's no reason to make copy of the shared_ptr when a const reference will do.


Also, on a bit of a design tangent, are you sure you need shared_ptr? I'm a bit afraid you've only used it because it's convenient to have copy semantics. If you only have one object or scope owning something, you can use a unique_ptr and pass it by const-reference instead of making copies. If you actually need shared ownership semantics, then yes, of course go with shared_ptr. If there is single ownership though, unique_ptr has much, much better performance than shared_ptr.

share|improve this answer
    
You are right about most of things. This my first project in C++, really. I took some courses 5+ years ago, but from then only highter level stuff. When I decided to used shared_ptr I was just baffled by the complexity of move semantics. Maybe a simple clarification is shared_ptr::operator-> slower than unique_ptr::operator->, or copying is more expensive. I also agree that I probably should use XYZHit&, but It should be irrevelant in case of performance as it is optimized out ;). I'll check it however. –  jb. Apr 13 at 22:08
    
@jb. Dereferencing should be the same speed for both shared_ptr and unique_ptr. Dereferencing should be just the same as dereferencing a normal pointer provided the compiler does its magic and inlines operator-> or operator* call. The performance hit comes from shared_ptr doing a lot more work than unique_ptr to keep track of meta data about the managed object. unique_ptr is just a glorified RAII container. –  Corbin Apr 13 at 23:23
    
shared_ptr, however, has to know how many copies of itself exist and only destruct the object when the count reaches 0 (and it has to 0 out weak_ptr). What complicates it even further is that shared_ptr supports multithreading, meaning incrementing and decrementing the count is not a simple add or sub, but instead requires a much slower atomic add/sub, quite a bit of extra work over unique_ptr's nothing. –  Corbin Apr 13 at 23:30
    
Thanks. Could you elaborate more on (mentioned in the other answer) possible problems with non explicit constructors. –  jb. Apr 14 at 17:19
    
@jb. I will edit in a more detailed explanation when I have time, but the short version of it is that 1 argument non-explicit constructors allow potentially unexpected conversions to happen at the cost of type safety. For example: struct S { S(int x); }; and void f(S s);. Would you expect f(3) to compile? It does because S can be implicitly constructed from an int. In some situations this is very useful and the semantics are pretty natural. In others, it can cause very subtle bugs. Due to the risk for misunderstandings, I tend to always avoid implicit constructors. –  Corbin Apr 15 at 8:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.