0

I am learning designing principles course and as I was trying to implement a open close principle on my own where we define different specification which is then sent to filter class to check whether the given entity is valid entity or not.

These are my virtual class

template <typename T>
struct Specification {
    virtual ~Specification() {}
    virtual bool is_satisfied(T* item) = 0;
};


template <typename T>
struct Filter {
    virtual std::vector<T*> filter(std::vector<T*> items, Specification<T>& spec) = 0;
    
};
template <typename T>
struct AndSpecification : Specification<T> {
    Specification<T>& first;
    Specification<T>& second;
    AndSpecification(Specification<T>&& first, Specification<T>&& second) : first(first), second(second) {}
    
    bool is_satisfied(T* item) override {
        return first.is_satisfied(item) && second.is_satisfied(item);
    }

};


struct BetterFilter : Filter<Product> {
    std::vector<Product* > filter(std::vector<Product*> items, Specification<Product>& spec);
};

struct ColorSpecification : Specification<Product> {
    Color color;
    ColorSpecification(Color color) :color(color) {}
    bool is_satisfied(Product* item);
};

struct SizeSpecification : Specification<Product> {
    Size size;
    SizeSpecification(Size size) :size(size) {}
    bool is_satisfied(Product* item);
};

These are my inherited classes

And based on these classes I am trying to override && operator which is defined in global scope. `

template <typename T>
AndSpecification<T> operator&&(Specification<T>&& one, Specification<T>&& two) {
    return AndSpecification<T>{one, two};
}

When I am trying to compile this I am getting error

Error C2440 '<function-style-cast>': cannot convert from 'initializer list' to 'AndSpecification<Product>' DesignPrinciples C:\Users\ania\source\repos\DesignPrinciples\Main.cpp 24

I tried different methods to define like

AndSpecification<Product> operator&&(Specification<Product>&& one, Specification<Product>&& two) {
    return AndSpecification<Product>(one, two);
}

but even then it failed. Can someone explain me what am i doing wrong here. How can I rectify it ?

Edit1: Thanks to @Yksisarvinen I am able to compile when I add std::move in template method

AndSpecification<T> operator&&(Specification<T>&& one, Specification<T>&& two) {
    return AndSpecification<T>{std::move(one), std::move(two)};
}

But when i run it, it fails with read access violation

I am getting this exception at this line

AndSpecification<Product> spec = ColorSpecification(Color::green) && SizeSpecification(Size::large);

Edit 2: Here is the minimal reproducible example.

Demo

Jason
  • 36,170
  • 5
  • 26
  • 60
  • `one` and `two` are lvalues, because they have names. You still need `std::move` on them to make them rvalues. – Yksisarvinen Oct 29 '22 at 20:19
  • The arguments to your `&&` override are r-values. You can only call them with r-values. Change them to `const &` – doug Oct 29 '22 at 20:20
  • Kinda related, searching for a better duplicate: [Why is a named rvalue reference an lvalue expression?](https://stackoverflow.com/questions/56316766/why-is-a-named-rvalue-reference-an-lvalue-expression) – Yksisarvinen Oct 29 '22 at 20:23
  • Well, that means you have Undefined Behaviour somewhere in your code. Might be related to your move operations, but you need to debug that yourself. – Yksisarvinen Oct 29 '22 at 20:34
  • 3
    Lots of references to temporaries; I think we need a [mcve] to check everything. – Richard Critten Oct 29 '22 at 20:35
  • 2
    in general don't overload that operator, as it can't work in the same way as the built-in one - use a named function – Neil Butterworth Oct 29 '22 at 21:51
  • I am sharing the complete codebase which i have written. Let me know if anything else is required ```https://onecompiler.com/cpp/3ymgw92a4``` – Animesh Srivastawa Oct 30 '22 at 19:31
  • @NeilButterworth: I assume you specifically mean the shortcut evaluation? That can be implemented. In this case, `AndCondition` indeed does not call `second.is_satisfied` if the first filter returns `false`. What it can't do is shortcut evaluation when the operator _itself_ is called. It will always combine the two filter expressions `one` and `two`. – MSalters Oct 31 '22 at 12:47
  • @MSalters yes i did mean that and stylistically i have never seen an overload of && or || that made much sense, and this is no exception – Neil Butterworth Oct 31 '22 at 12:55

1 Answers1

0

C++ is not garbage-collected. If you're using pointers, you need to keep track of ownership. If you're storing references, you need to make sure that the reference refers to an object that is still alive.

AndCondition fails in this regard. It stores two references, but has no way to know if the objects referenced are still alive.

std::unique_ptr<> could be a workable solution. A std::unique_ptr keeps the unique (sole) ownership of an object, and supports derived classes.

A std::unique_ptr can be moved - this is by design. This transfers ownership. Thus, AndSpecification(std::unique_ptr<Specification<T>>&& first, std::unique_ptr<Specification<T>>&& second) makes perfect sense. This takes ownership.

MSalters
  • 173,980
  • 10
  • 155
  • 350