google
Options::Options() constructor is defined without a matching destructor
At https://github.com/google/draco/blob/c0b57c1b6ec365a5e97f765deacb6b45ead104a3/src/draco/core/options.cc#L23 You can see the Options::Options() constructor is instantiated. As soon as constructor is here it would be great to instantiate the destructor as well. Just add
Options::~Options();
and the destructor declaration.
For example, It would help when DLL and application using it are compiled with different Microsoft STL versions (slightly different debug iterators). In other words the minor change will made a Draco dll more binary compatible.
This will be fixed in the next release, which does not yet have a planned date. It has been fixed by removing the empty constructor, and then explicitly defaulting the Options constructor and destructor.
Defaulting constructor and destructor is great! Just do it in the options.cc and not in a header file!
It's in the header file, as required. Besides that, how would one "= default" a constructor in a .cc file? How would that be useful to other modules that cannot see the definition?
Anyway, I think I'm misunderstanding the issue here. Defaulting a constructor outside the class declaration is an error, per clang:
Out-of-line definition of 'Test' does not match any declaration in 'test::Test'clang(member_decl_does_not_match)
The internal version of Options now looks like this:
class Options {
public:
Options() = default;
~Options() = default;
...
};
edit: Forgot the public, added formatting.
See the fifth variant of default constructing at https://en.cppreference.com/w/cpp/language/default_constructor#:~:text=A%20default%20constructor%20is%20a,public%20default%20constructor%20is%20DefaultConstructible. In header file
class Options {
public:
Options();
~Options();
...
};
in options.cc:
Options::Options()=default;
Options::~Options()=default;
It guarantees that both constructor and destructor are created at the library compile time. In turn it means better binary compatibility of the public API across different compiler versions/settings.
I was not aware of the 5th form: thanks for that.
I need some more information on why this is necessary. Defaulting ctors/dtors with the common syntax is a very widely used pattern, and is used in multiple places within Draco:
$ git grep '= default;'
src/draco/attributes/attribute_transform.h:29: virtual ~AttributeTransform() = default;
src/draco/attributes/attribute_transform_data.h:33: AttributeTransformData(const AttributeTransformData &data) = default;
src/draco/attributes/point_attribute.h:40: PointAttribute(PointAttribute &&attribute) = default;
src/draco/attributes/point_attribute.h:41: PointAttribute &operator=(PointAttribute &&attribute) = default;
src/draco/compression/attributes/attributes_decoder.h:35: virtual ~AttributesDecoder() = default;
src/draco/compression/attributes/attributes_decoder_interface.h:33: AttributesDecoderInterface() = default;
src/draco/compression/attributes/attributes_decoder_interface.h:34: virtual ~AttributesDecoderInterface() = default;
src/draco/compression/attributes/attributes_encoder.h:34: virtual ~AttributesEncoder() = default;
src/draco/compression/attributes/kd_tree_attributes_decoder.cc:44: PointAttributeVectorOutputIterator &&that) = default;
src/draco/compression/attributes/points_sequencer.h:30: virtual ~PointsSequencer() = default;
src/draco/compression/attributes/prediction_schemes/prediction_scheme_interface.h:29: virtual ~PredictionSchemeInterface() = default;
src/draco/compression/attributes/sequential_attribute_decoder.h:29: virtual ~SequentialAttributeDecoder() = default;
src/draco/compression/attributes/sequential_attribute_encoder.h:30: virtual ~SequentialAttributeEncoder() = default;
src/draco/compression/mesh/mesh_edgebreaker_decoder_impl_interface.h:30: virtual ~MeshEdgebreakerDecoderImplInterface() = default;
src/draco/compression/mesh/mesh_edgebreaker_encoder_impl_interface.h:35: virtual ~MeshEdgebreakerEncoderImplInterface() = default;
src/draco/compression/mesh/traverser/traverser_base.h:33: virtual ~TraverserBase() = default;
src/draco/compression/point_cloud/point_cloud_decoder.h:31: virtual ~PointCloudDecoder() = default;
src/draco/compression/point_cloud/point_cloud_encoder.h:32: virtual ~PointCloudEncoder() = default;
src/draco/core/decoder_buffer.h:33: DecoderBuffer(const DecoderBuffer &buf) = default;
src/draco/core/decoder_buffer.h:35: DecoderBuffer &operator=(const DecoderBuffer &buf) = default;
src/draco/core/status.h:39: Status(const Status &status) = default;
src/draco/core/status.h:40: Status(Status &&status) = default;
src/draco/core/status.h:54: Status &operator=(const Status &) = default;
src/draco/core/status_or.h:35: StatusOr(const StatusOr &) = default;
src/draco/core/status_or.h:36: StatusOr(StatusOr &&) = default;
src/draco/io/file_reader_factory.h:19: ~FileReaderFactory() = default;
src/draco/io/file_reader_factory_test.cc:25: // ~AlwaysFailFileReader() override = default;
src/draco/io/file_reader_factory_test.cc:50: ~AlwaysOkFileReader() override = default;
src/draco/io/file_reader_factory_test.cc:63: AlwaysOkFileReader() = default;
src/draco/io/file_reader_interface.h:12: FileReaderInterface() = default;
src/draco/io/file_reader_interface.h:16: FileReaderInterface(FileReaderInterface &&) = default;
src/draco/io/file_reader_interface.h:17: FileReaderInterface &operator=(FileReaderInterface &&) = default;
src/draco/io/file_reader_interface.h:20: virtual ~FileReaderInterface() = default;
src/draco/io/file_writer_factory.h:19: ~FileWriterFactory() = default;
src/draco/io/file_writer_factory_test.cc:25: // ~AlwaysFailFileWriter() override = default;
src/draco/io/file_writer_factory_test.cc:44: ~AlwaysOkFileWriter() override = default;
src/draco/io/file_writer_factory_test.cc:49: AlwaysOkFileWriter() = default;
src/draco/io/file_writer_interface.h:10: FileWriterInterface() = default;
src/draco/io/file_writer_interface.h:14: FileWriterInterface(FileWriterInterface &&) = default;
src/draco/io/file_writer_interface.h:15: FileWriterInterface &operator=(FileWriterInterface &&) = default;
src/draco/io/file_writer_interface.h:18: virtual ~FileWriterInterface() = default;
src/draco/io/stdio_file_reader.h:26: StdioFileReader(StdioFileReader &&) = default;
src/draco/io/stdio_file_reader.h:27: StdioFileReader &operator=(StdioFileReader &&) = default;
src/draco/io/stdio_file_writer.h:24: StdioFileWriter(StdioFileWriter &&) = default;
src/draco/io/stdio_file_writer.h:25: StdioFileWriter &operator=(StdioFileWriter &&) = default;
src/draco/material/material_library.h:34: MaterialLibrary() = default;
src/draco/point_cloud/point_cloud.h:32: virtual ~PointCloud() = default;
src/draco/scene/instance_array.h:40: InstanceArray() = default;
src/draco/texture/texture_map.h:107: TextureMap(TextureMap &&) = default;
src/draco/texture/texture_map.h:142: TextureMap &operator=(TextureMap &&) = default;
What situation is this addressing?
In turn it means better binary compatibility of the public API across different compiler versions/settings.
Is there any information supporting this statement?
edit: Added the grep output showing the existing usage in current HEAD.
The Options problem happened because constructor and destructor were generated by different compiler versions with slightly different STL (Microsoft debug iterators). Constructors/destructors dealing with STL objects (std::map in the Options class) are quite vulnerable to such discrepancies in build environments.
The situation is not a reason to make so many changes in multiple files. It's more like my own fault (not recompiling libraries when migrating to a new compiler/STL). Sorry for spending your time.
P.S.: I still think that for non trivial classes that contain STL members (exposed in a public API) it may have sense to define all constructors and destructors in c* files rather than in headers. If possible