Make some improvements around exception handling and documentation fixes #50
Conversation
| @@ -19,7 +19,7 @@ class Exception : public std::exception { | |||
| Exception(const Exception &ex) | |||
| : std::exception(), filename_(ex.filename_), line_(ex.line_), | |||
| error_code_(ex.error_code_), error_message_(ex.error_message_) {} | |||
| virtual ~Exception() throw() {} | |||
| virtual ~Exception() noexcept; | |||
There was a problem hiding this comment.
noexcept is 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.
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.
I believe that throw() is also C++11.
It looks like it's C++98. See exception-specification: https://www.lirmm.fr/~ducour/Doc-objets/ISO+IEC+14882-1998.pdf
Do you still want this #if'd?
My opinion is totally unimportant and irrelevant for getting this merged, but it seems like either #if or raise the required C++ version.
There was a problem hiding this comment.
Experimentally confirmed that noexcept requires c++11.
Try this with various -std args:
#if __has_feature(cxx_noexcept)
#error yes noexcept
#else
#error no noexcept
#endif
The current version of the PR doesn't compile for me:
libtool: compile: g++ -DPACKAGE_NAME=\"marisa\" -DPACKAGE_TARNAME=\"marisa\" -DPACKAGE_VERSION=\"0.2.6\" "-DPACKAGE_STRING=\"marisa 0.2.6\"" -DPACKAGE_BUGREPORT=\"[email protected]\" -DPACKAGE_URL=\"\" -DPACKAGE=\"marisa\" -DVERSION=\"0.2.6\" -DHAVE_STDIO_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_STRINGS_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_UNISTD_H=1 -DSTDC_HEADERS=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -I. -march=native -Wall -Weffc++ -Wextra -Wconversion -I../../../../include -I../../../../lib -g -O2 -DMARISA_USE_POPCNT -mpopcnt -DMARISA_USE_SSE4 -msse4 -MT mapper.lo -MD -MP -MF .deps/mapper.Tpo -c mapper.cc -fno-common -DPIC -o .libs/mapper.o
...
../../../../include/marisa/exception.h:22:11: error: exception specification of overriding function is more lax than base version
virtual ~Exception() noexcept;
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/exception:101:13: note: overridden virtual function is here
virtual ~exception() _NOEXCEPT;
^
In file included from mapper.cc:13:
In file included from ../../../../lib/marisa/grimoire/io/mapper.h:6:
In file included from ../../../../include/marisa/base.h:191:
../../../../include/marisa/exception.h:22:23: error: expected ';' at end of declaration list
virtual ~Exception() noexcept;
% g++ --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/usr/include/c++/4.2.1
Apple clang version 12.0.0 (clang-1200.0.32.29)
Target: x86_64-apple-darwin19.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
| * | ||
| * 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.
| * 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. | ||
| * 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. libmarisa.a is about 10k smaller when ~Exception is defined in exception.cc. The size difference goes away when it's linked, and the final static tools/ binaries are a few 10s of bytes smaller. libmarisa.dylib is also the same size.
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 noexcept, not why it's defined here.
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.
The main change is around how the Marisa exception is defined. See exception.cc for the full reasoning behind the change.
The rest are just documentation clean up changes in the HTML files. There were some markup validity issues, and using https is generally a good idea.