Some safety improvement changes

[Deamhan]

wxSQLite3StringCollection::Bind should return if m_data is nullptr otherwise it will fail. Sign/length mismatch was fixed in chacha20_xor.

[utelle]

Regarding wxSQLite3StringCollection::Bind and wxSQLite3IntegerCollection::Bind you are right that both methods can fail resp crash if m_data == nullptr. However, if used correctly m_data == nullptr should never be true.

Nevertheless this case should be handled, of course, although I'm not convinced that simply returning from the Bind methods is the right approach. IMHO it would be better to throw an exception if m_data == nullptr. Otherwise the application would assume that method Bind was executed successfully, possibly leading to obscure bugs later on.

[Deamhan]

Yes, I agree with you, exception throwing is a good idea.

[Deamhan]

I started digging into wxSQLite3NamedCollection and found than default constructor creates damaged object (no methods to correct it) so I removed it. I looked existing exceptions and didn't find something like "corrupted object state".

[utelle]

The problem here is that we need a default constructor. And if a default constructor is not explicitly defined, the compiler will implicitly define one - that's why the sample application is built and run successfully.

Maybe a method IsOk should be added, so that the validity of an instance can be checked by the application. It is still necessary to add throwing exceptions in the Bind methods.

Regarding the exception I would use a wxSQLite3Exception exception with a generic error code and an explanatory message.

[Deamhan]

Done. About default constructor: it is true only if you don't define any alternative (with parameters or not) except move/copy constructors (https://en.cppreference.com/w/cpp/language/default_constructor). Now attempt to build wxSQLite3NamedCollection or derivatives will fail.

[utelle]

Just a few remarks:

1. I would prefer method name IsOk over IsGood, because I'd like to be consistent with wxWidgets. And in wxWidgets always method name IsOk is used for this purpose (at least as far as I know).
2. Please use the error code WXSQLITE_ERROR (instead of -1) for throwing exceptions in the Bind methods.
3. Please define a constant for the error message string as is done for all other wxSQLite3 specific error messages.
4. IMHO adding member m_good isn't necessary, because checking m_data already allows to test whether the state of the object is ok or not.
5. IMHO defining a default constructor is required, unless we want to force developers to always initialize a collection instance with proper values on declaring a collection object. Removing the default constructors could possibly break existing code. This should be avoided.
6. Please remove keyword throw from method declarations (AFAIK deprecated in C++11 and will be removed in C++20).

[Deamhan]

1. I agree, it will definitely better, corrected.
2. Done. But there is other places with -1 as code, may be we should correct it?
3. Done.
4. It was done for future usage, but you are right, it is almost always a bad practice. Done.
5. I don't really understand purpose of definitely corrupted objects. There is no methods to fix it, only assignment... But if you insist on recovering default constructors, I will do it.
6. Done. I'm a fan of noexcept and exception-free coding at all, but this project is written according to c++98, so I used throw() instead of noexcept.

[utelle]

Thanks for applying most of my proposed modifications. PR looks good now.

2. But there is other places with -1 as code, may be we should correct it?

This is on purpose, because the errors are not related to SQLite, but to loading a DLL.

Personally, I no longer use the option to separate the wrapper, wxSQLite3, from the SQLite library itself by dynamically loading a SQLite DLL. Using a SQLite DLL makes it difficult to keep the supported features in sync between the wrapper and the SQLite library. Therefore the preferred method is to link the SQLite library statically with the wrapper. Most likely I will remove the option to dynamically load a SQLite DLL in a future version. However, I will not do it without asking for opinions from users of the component.

5. I don't really understand purpose of definitely corrupted objects. There is no methods to fix it, only assignment... But if you insist on recovering default constructors, I will do it.

Well, in one of my applications I have a class using collections where the collection objects are members of the class. However, it is not always possible to initialize the collection objects on creation of a class instance, because the actual database is not yet known. Therefore I fear it is necessary to have default constructors at hand.

6. I'm a fan of noexcept and exception-free coding at all, but this project is written according to c++98, so I used throw() instead of noexcept.

Yes, the code of wxSQLite3 has not been moved to using C++11 (or above) yet, since there exist several legacy applications based on C++98. However, I intend to upgrade the wxSQLite3 code to a more recent C++ version in the future - no fixed schedule yet, though.

Regarding exceptions I was asked several times in the past, whether it would be possible to provide an exception free version of wxSQLite3. However, it would be a major change to abandon exceptions from wxSQLite3, and I'm not convinced that that would really be a desirable approach.

[Deamhan]

5. Done

in one of my applications I have a class using collections where the collection objects are members of the class. However, it is not always possible to initialize the collection objects on creation of a class instance, because the actual database is not yet known

I recommend unique_ptr or raw pointer, or even placement new in this cases.

But I have a question: does wxSQLite3NamedCollection should have a public constructor? As I see it is just a base class only for internal purposes, am I right? Maybe we should prevent its direct building?

[utelle]

I have a question: does wxSQLite3NamedCollection should have a public constructor? As I see it is just a base class only for internal purposes, am I right? Maybe we should prevent its direct building?

Yes, wxSQLite3NamedCollection is just the base class for the currently provided collection types, wxSQLite3IntegerCollection and wxSQLite3StringCollection. The class can't be used on its own. So, it could be maybe transformed into an abstract base class.

[Deamhan]

wxSQLite3NamedCollection::wxSQLite3NamedCollection() is protected now, so wxSQLite3NamedCollection objects can not be constructed any more. All proposed modifications is done as I see. What do you think?

[utelle]

Thanks for your efforts. I'm now going to merge your PR.

[Deamhan]

Thank you for a constructive dialogue.