pcl::PCLException shall have a nothrow copy constructor and a nothrow copy assigment operator

[soyersoyer]

I think the PCLException class should

• uses some reference counting technique for storing the strings,
• or stores the file_name and function_name as const char* and stores the (detailed) message in the std::runtime_error.

What do you think?
Does it makes sense to call the getLineNumber(), getFunctionName(), getFileName() on an exception?
The what() and the detailedMessage() should return the same message?

§18.8.1/2: "Each standard library class T that derives from class exception shall have a publicly
accessible copy constructor and a publicly accessible copy assignment operator that do not exit with an exception. These member functions shall meet the following postcondition: If two objects lhs and rhs both have dynamic type T and lhs is a copy of rhs, then strcmp(lhs.what(), rhs.what()) shall equal 0."
"When the exception handling mechanism, after completing the initialization of the exception object
but before activation of a handler for the exception (15.1), calls a function that exits via an exception, std::terminate is called (15.5.1)."

[taketwo]

Could you please elaborate a bit more on your proposals?

use some reference counting technique for storing the strings

Why? What (and how much) can we gain from this?

or stores the file_name and function_name as const char*

How is this better than currently used std::string? Or do you mean that we should add const qualifiers?

Does it makes sense to call the getLineNumber(), getFunctionName(), getFileName() on an exception?

Have no clue actually, but these function do exist and may be used by existing user code. Removing them will break API with no added benefit.

The what() and the detailedMessage() should return the same message?

From what I see, they do return the same message.

As per the title of the issue, I think PCLException has both. They are implicitly generated by the compiler.

[soyersoyer]

The 'problem' is that the compilers may generate code which copies the exception after throwing that exception. (and before catching)
When something throws between throwing and catching, the std::terminate is called.

The std::string's copy constructor and assignment operator may throw (because of the heap allocation), so the same functions in PCLException may throw too.

Reference counting:
If the strings allocated on the heap and the PCLException just stores the pointers with reference counting (for example in a shared_ptr), the copy constructor and the assignment operator can be nothrow.(because copying a shared_ptr is nothrow)
Some of the std::runtime error's library implementations use this technique, some store a fixed sized char[].

One simpler solution:
The FILE and the BOOST_CURRENT_FUNCTION is a const char* and storing them as const char* is ok. (copying is ok too)
The runtime_error's constructor accepts one string, so the message can be stored there.

[soyersoyer]

more info:
https://www.securecoding.cert.org/confluence/display/cplusplus/ERR14-CPP.+Do+not+allow+an+exception+class%27s+copy+constructor+to+throw+exceptions

It's not a big issue, because today's compilers don't copy exceptions (clang 3.4, gcc 4.9, vs2013), except when the user want (eg throw e;). But older compilers might copy.

[taketwo]

The __FILE__ and the BOOST_CURRENT_FUNCTION is a const char* and storing them as const char* is ok. (copying is ok too)

+1

The detailedMessage () function can throw, so it should not be called in the constructor. Therefore we may remove message_ member completely. But we should not call detailedMessage() from what() either, right?

[soyersoyer]

It is not a problem if an exception's constructor throws. The constructed exception is lost (it never existed) and the new one is handled instead. So calling detailedMessage in the constructor is not a problem.
The what() should be nothrow (because it is virtual and nothrow in std::exception).
The std::runtime_error has a what(), which returns the message from the runtime_error's constructor.

[taketwo]

It is not a problem if an exception's constructor throws. The constructed exception is lost (it never existed) and the new one is handled instead.

But this is misleading, isn't it? Instead of getting a PCLException we receive some weird bad_alloc...

Still, I am not sure I understand everything you propose. Changing string to const char* is fine. Anything else?

[soyersoyer]

Yes, it is misleading, but it is better than a pure std::terminate call.
If you throw std::runtime_error and construct a string to pass to its constructor, you might get bad_alloc too.

The changes:
soyersoyer@939fc02

[taketwo]

Looks good to me! Just add a space in line 102 ;)

[SergioRAgostinho]

Fixed in #1119