Smart pointers proper usage in my project

[frek]

I'm doing an exercise of Bjarne's book PPP (chapter 21):

6. In the Fruit example in §21.6.5, we copy Fruits into the set. What if we didn’t want to copy the Fruits? We could have a set<Fruit*> instead. However, to do that, we’d have to define a comparison operation for that set. Implement the Fruit example using a set<Fruit*, Fruit_comparison>. Discuss the differences between the two implementations.

For that I 've written the code below for now:

#include <iostream>
#include <string>
#include <set>

struct Fruit {
	std::string name;
	int count;
	double unit_price;
};

bool operator<(const Fruit& f1, const Fruit& f2) {

	// Compare the Fruits by their unit prices
	return f1.unit_price < f2.unit_price;
}

std::ostream& operator<<(std::ostream& os, const Fruit* f) {
	return os << f->name << ' ' << f->count << ' ' << f->unit_price << '\n';
}

//********************************************************************

int main()
{
	std::set<Fruit*> fSet{ new Fruit {"Apple", 20, 28.9}, new Fruit{ "Orange", 12, 43.2 }
		, new Fruit{"Banana", 42, 39.87}, new Fruit {"Melon", 15, 16.8} };

	for (const auto& s : fSet)
		std::cout << s;

	system("pause");
	return 0;
}

It's simple and works fine. A couple of questions:

1) I think it's time to delve into smart pointers and use a unique_ptr for fSet here to bow out of raw pointers for which in the code above I even didn't know how to properly release the heap allocated. How to properly replace the raw pointer with a smart pointer in this example, please?

2) The exercise says that we define: set<Fruit*, Fruit_comparison>. I've provided the set with a predicate (line 11 which surprisingly sorts the data de-ascendingly in lieu of ascendingly! Hhhh) but that "Fruit_comparison" seems to be the name of a functor or like that.

A side question about std::set: we say it doesn't provide the subscript operator ([]) because it lacks random iterators and therefore using that operator will dramatically decrease the performance. Right?

[keskiverto]

It's simple and works fine.

a predicate (line 11 which surprisingly sorts the data de-ascendingly in lieu of ascendingly! Hhhh)

That is hardly "works" nor "fine".

Add a cout into your predicate to see how often it is called.

If the task is to write set<Fruit*, Fruit_comparison>, then set<Fruit*> is not ok.

#include <iostream>
#include <string>
#include <set>

struct Fruit {
	std::string name;
	int count;
	double unit_price;
};

std::ostream& operator<<(std::ostream& os, const Fruit& f) {
	return os << f.name << ' ' << f.count << ' ' << f.unit_price << '\n';
}

//********************************************************************

struct comp {
    bool operator()(const Fruit* f1, const Fruit* f2)
    { return f1->unit_price < f2->unit_price; }
};

int main()
{
	std::set<Fruit*,comp> fSet{ new Fruit {"Apple", 20, 28.9}, new Fruit{ "Orange", 12, 43.2 }
		, new Fruit{"Banana", 42, 39.87}, new Fruit {"Melon", 15, 16.8} };

	for (const auto& s : fSet)
		std::cout << *s;
}

in the code above I even didn't know how to properly release the heap allocated.

You have a collection of pointers to dynamically allocated objects and you don't know how to deallocate those objects?

for ( auto pointer : fSet ) {
  delete pointer;
}

[coder777]

1) I would think it's not really worth the hassle using unique_ptr which limits the access...

2) The operator<(...) on line 11 will not be called because it expects references but your set contains pointers. Thus the content of the set is sorted according the value of the pointer which is rather arbitrary and probably according its creation.

[frek]

@keskiverto
I get the following error for your code on my vs 2019 compiler:
expression having type 'const comp' would lose some const-volatile qualifiers in order to call 'bool comp::operator ()(const Fruit *,const Fruit *)' 1614

You have a collection of pointers to dynamically allocated objects and you don't know how to deallocate those objects?

For a moment I mixed up using delete [] fSet and your version! :(

@coder777

I would think it's not really worth the hassle using unique_ptr which limits the access ...

What do you mean by "limit the access", please?
As well as, I wanted to use a smart pointer just to learn how to use them for that project not necessarily use them there in practice.

The operator<(...) on line 11 will not be called because it expects references but your set contains pointers.

That's strange when I declare the comparison operator this way:
bool operator<(const Fruit* f1, const Fruit* f2)
I get this error:
'operator <' must have at least one formal parameter of class type 11

A side question:
In this snipped code,

std::ifstream inf(infile);
std::ofstream outf(outfile);

if (!inf || !outf) error("bad file name"); 

I get this error, even though I've included <exceptions>:
'error': identifier not found
What other header file should I include in order to use error that way?

[keskiverto]

frek wrote:

my vs 2019 compiler:
expression having type 'const comp' would lose some const-volatile qualifiers in order to call 'bool comp::operator ()(const Fruit *,const Fruit *)' 1614

True, I forgot const-correctness and the test platform did not say a word about that. Verbose compiler is good.

Lets try more explicit:

	Fruit* x {};
	Fruit* y {};
	const comp test;
	test( x, y );

 In function 'int main()':
error: no match for call to '(const comp) (Fruit*&, Fruit*&)'
note: candidate is:
note: bool comp::operator()(const Fruit*, const Fruit*) <near match>
note:   no known conversion for implicit 'this' parameter from 'const comp*' to 'comp*'

Can you decipher the error messages?

[lastchance]

@Frek
Just change
bool operator()(const Fruit* f1, const Fruit* f2)
to
bool operator()(const Fruit* f1, const Fruit* f2) const


However, I tried this on the succession of C++ compilers in Wandbox and the original worked up to C++14
https://wandbox.org/permlink/3bs5WAyGytaUuPiJ
but then failed from C++17 onward:
https://wandbox.org/permlink/zeHpaGifWYKGDZZL

Did the standard change this, or was it simply not being respected before? There are going to be a number of books and example sites made completely out of date by this.

This level of "const-correctness" and verbosity is something that is killing the language.

[coder777]

What do you mean by "limit the access", please?

unique_ptr cannot be copied. Hence things like the initializer list on line 25 are not possible.

That's strange when I declare the comparison operator this way:

The global operator obviously does not accept pointer. It requires another form as keskiverto showed.

What other header file should I include in order to use error that way?

error does not exists in the standard library. What is it supposed to do?

For <exception> see:

http://www.cplusplus.com/reference/exception/

[frek]

@keskiverto

Can you decipher the error messages?

No. This is the first time I'm about to decipher these kinds of errors. So, firstly let me decipher my previous error message and if I succeed in deciphering it correctly, you give me "one" more error message as my next deciphering step, please.
expression having type 'const comp' would lose some const-volatile qualifiers in order to call 'bool comp::operator ()(const Fruit *,const Fruit *)' 1614

It says, there's some expression which has type 'const comp' which loses some qualifier of type 'const-volatile' (?) when used to call 'bool comp::operator ()(const Fruit *,const Fruit *)'.
On the other hand, from https://en.cppreference.com/w/cpp/named_req/BinaryPredicate, we have this line: In addition, evaluation of that expression is not allowed to call non-const member functions of the dereferenced iterators.. It seems that the user-defined predicate function must have a const qualifier at the end or its declaration. Stroustrup has also used it that way in the chapter, but it's hard to remember each and every word of the chapter's script! :(

Now how was my deciphering? If it's correct then I'm ready for more error messages to decipher, of course one-by-one.

@coder777

unique_ptr cannot be copied. Hence things like the initializer list on line 25 are not possible.

Yes, for that initializer list the copy constructor is called. But what if I forget to call delete for every element of the class set and we want to use a shared_ptr to both bypass copying limitation and free memory as well? How to use this smart pointer in the project?

The global operator obviously does not accept pointer.

Does it mean that any global operator function doesn't accept any of its parameters in type pointer?

For <exception> see:
http://www.cplusplus.com/reference/exception/

I've already included that header but the error still exists!

[lastchance]

@Frek, are you getting confused with perror:
http://www.cplusplus.com/reference/cstdio/perror/

Otherwise you should be throwing a specific exception, not using a non-existent (in c++) function.

[coder777]

How to use this smart pointer in the project?

Why don't you just try it yourself?
Here is an example with std::shared_ptr:

#include <iostream>
#include <string>
#include <set>
#include <memory>

struct Fruit {
	std::string name;
	int count;
	double unit_price;

	Fruit(std::string n, int c, double p) : name{n}, count{c}, unit_price{p}
	{
	}
};

std::ostream& operator<<(std::ostream& os, const std::shared_ptr<Fruit> f) {
	return os << f->name << ' ' << f->count << ' ' << f->unit_price << '\n';
}

//********************************************************************

int main()
{
	std::set<std::shared_ptr<Fruit>> fSet{ std::make_shared<Fruit>("Apple", 20, 28.9), std::make_shared<Fruit>("Orange", 12, 43.2)
		, std::make_shared<Fruit>("Banana", 42, 39.87), std::make_shared<Fruit>("Melon", 15, 16.8) };

	for (const auto s : fSet)
		std::cout << s;

	system("pause");
	return 0;
}

std::unique_ptr would work too, but needs a bit more effort.

Does it mean that any global operator function doesn't accept any of its parameters in type pointer?

It means that such a global operator does not accept primitive (also known as pod) types (like pointer, int, etc) only. As soon as there is at least one parameter of a user defined type the compiler won't throw an error.

I've already included that header but the error still exists!

That is because error(...) doesn't exists. Not in <exception> or else where in the standard library. See the link for what exists in <exception>.

[keskiverto]

It seems that the user-defined predicate function must have a const qualifier at the end or its declaration.

Yes. A member function can be const-qualified.
The implementation of such function cannot modify the object (*this) and therefore it can be called on immutable objects.
It is possible to have overloaded (non-const and const) members.

struct T {
  void example();
  void sample() const;
  void other();       // #1
  void other() const; // #2
}

const T x;
x.example(); // error: x is const
x.sample();  // ok
x.other(); // ok, calls #2

T y;
y.example(); // ok
y.sample();  // ok
y.other(); // ok, calls #1 

Apparently latest standard libraries have const comparison object.
That constness means that when the set calls comp( f1, f2 ), the comp will not change.

[seeplus]

Did the standard change this, or was it simply not being respected before? There are going to be a number of books and example sites made completely out of date by this.

AFAIK (and others may know the standards better than I do), this was a standard change in C++17. There was a general tightening of const. Yes, this is a problem with some sites/books. They are out of date as soon as a new C++ standard is released. - and sites often aren't updated. There's also changes in C++20 to make things like std::string etc constexpr . We had compile issues when we moved our code base from C++14 to C++17...

[frek]

@lastchance
No, Stroustrup used it in the TEA algorithm but now I think it's a typo. I will throw an exception instead of that.

@coder777
I tired but didn't know that I should have used "make_shared" too. Probably when we want to initialize or assign to a shared_ptr object we need to use make_shared that way.

It means that such a global operator

One example of "such" functions is a predicate. What other ones do we have to have such a specification?

[Niccolo]

@frek, @coder777

make_shared has an interesting purpose.

Without it, a "new" object held by a shared_ptr has two allocations. The first is for a "node", merely an object the shared_ptr uses to contain the underlying object, and the one which counts the references (the number of shared_ptrs and weak_ptrs that hold the node).

The second allocation is for the object begin created.

However, make_shared unifies these two different pieces into one single allocation, placing the new object in the latter portion of the memory. This saves some time and a little bit of memory each time it is used.

In older work, this was the reason for intrusive nodes. Some may still use them, but the idea is that in order to unify these two separate concepts, an intrusive approach requires one to derive the class to be "newed" from a node base, which holds the reference count. In this way one allocation creates both components.

As a result, make_shared provides the efficiency improvement of intrusive containment without intrusion.