Make some improvements around exception handling and documentation fixes #50
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| #include "marisa/exception.h" | ||
|
|
||
| namespace marisa { | ||
|
|
||
| /** | ||
| * This is defined here for the following reasons. | ||
| * | ||
| * 1. Virtual destructors are best not defined in a header to be included in many compilation units. | ||
| * Defining it here in a single compilation unit prevents multiple vtables from being generated and causing | ||
| * a compiler to be confused as to which is the true vtable, like with full link time optimization. | ||
|
There was a problem hiding this comment. Doesn't the linker just merge the vtables? What goes wrong? There was a problem hiding this comment. From my experience, no not really. Especially not when you use one in a shared library and then you include the same header in your application using that shared library. You get symbol conflicts. AIX is very finicky and has been vocal about name conflicts like this in the past when linking. When you're using clang and using full link time optimization, I believe that it will collapse the vtables. Unfortunately when you try to cast a pointer to determine if the object is of a specific subclass, it may not use the collapsed table. This caused a lot of problems in our code in the past. So we use the -Wweak-vtables warning to avoid this issue in the future. That warning complains about this specific class, and we don't want to see this warning in our code. |
||
| * By reducing the number of vtable instances, you also reduce the size of your library or application. | ||
|
There was a problem hiding this comment. Did you measure a size difference? There was a problem hiding this comment. I don't have the specific number any more, but it was non-zero. There was a problem hiding this comment. Interesting. I'll have to try. There was a problem hiding this comment. I just tried on OS X. I guess some sufficiently bad linker must exist somewhere though. |
||
| * You can see warnings around this with the -Wweak-vtables compiler option. Such issues may not be seen till runtime. | ||
| * Using the default keyword on the destructor is the same as defining it in the header. | ||
| * | ||
| * 2. Exceptions can not be thrown from destructors. When an exception is thrown from a destructor | ||
|
There was a problem hiding this comment. This seems to be an explanation of why it's There was a problem hiding this comment. True. Point 1 is mostly around why it's in this file. Point 2 is more about why I changed the signature. I can eliminate point 2 if you don't think it's helpful. There was a problem hiding this comment. Some source code static analysis tools complain about the throws usage on destructors. There was a problem hiding this comment. Seems like something to be explained in the PR, not at the site of every destructor definition. There was a problem hiding this comment. This is the only destructor in this project with this issue. There was a problem hiding this comment. I also wanted the reasoning to be located with the code so that it doesn't get reverted by someone else's changes. |
||
| * while the stack is unwinding from another thrown exception, then the program will crash instead | ||
| * of throwing the exception. See https://isocpp.org/wiki/faq/exceptions#ctor-exceptions | ||
| */ | ||
| Exception::~Exception() noexcept {} | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noexceptis C++11. I seem to recall that @s-yata likes to keep this compatible with C++03, so maybe add some#if __cplusplus >= ...?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that throw() is also C++11. So I don't think this is changing the C++ level. It's also worth mentioning that throw() was deprecated in C++17 and removed in C++20. So there isn't a good reason to state that a destructor should throw exceptions.
https://en.cppreference.com/w/cpp/language/noexcept_spec
Do you still want this #if'd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it's C++98. See
exception-specification: https://www.lirmm.fr/~ducour/Doc-objets/ISO+IEC+14882-1998.pdfMy opinion is totally unimportant and irrelevant for getting this merged, but it seems like either
#ifor raise the required C++ version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Experimentally confirmed that
noexceptrequires c++11.Try this with various
-stdargs:The current version of the PR doesn't compile for me: