Change in legacy etl::bitset with nullptr construction (or how exactly is nullptr construction defined?) + error handling

[TG-SAG]

I'm trying to switch from an older ETL Version (18.7.0) to a newer Version (20.38.10) and I recognized a change in behavior if the etl::bitset is initialized with a nullptr. For example:

const char* s = nullptr;
etl::bitset<31> bs( s );
auto result = b.count();

In the old version the result was 0 because the etl::bitset was reset()'ed ("zero initialized"). In the newer version it depends. This is because in der old version ETL did an ETL_ASSERT() in the set() method. The new version does an ETL_ASSERT_OR_RETURN_VALUE() in the set() method. Because the new version returns from the set() method the initialization of the etl::bitset does not finish and the status of the class is basically not defined. Additionally the ETL_ASSERT's were empty if it was compiled with NDEBUG for a release build. Now this is depended on ETL_NO_CHECKS. This is first a behavioral change because of NDEBUG and second there shouldn't be a difference in behavior depending on ETL_NO_CHECKS. An Compiler Explorer example that shows the behavior: https://gcc.godbolt.org/z/4KEWr7W81 (legacy bitset, legacy bitset + ETL_NO_CHECKS, new bitset (from left to right))

1. What should happen if etl::bitset is initialized with a nullptr?
2. When should a method use ETL_ASSERT_OR_RETURN*?
3. Under which conditions should theETL_ASSERT's be ETL_DO_NOTHING?
4. Is it OK to have different behavior if ETL_NO_CHECKS is defined or not defined?

First I will state my view on these topics but I want to discuss it in the comments.

[1] The etl::bitset should be in a defined state. My personal opinion is that it should be in a defined state and this state is the initialization of all bits to zero. The ETL is made for embedded systems and I don't think that is practical to check for nullptr every time. And I also think - in comparision to the STL - there is most of the time no exception handling available to handle this case. This is already how the new etl::bitset handles this because the source checks for a nullptr and if this is the case it initializes the class via reset() (Interestingly there is no ETL_ASSERT anymore).

[2] When there is undefined behavior defined ;-) If the documentation says it's undefined behavior then it is allowed to return from "whatever is currently happening" and leave the class in an inconstent state (or open a gate to hell, whatever you prefer). Because of my answer to [1] it shouldn't be undefined behavior and therefor not just return without finishing initialization. Perhaps someone can elaborate on why the *_RETURN_* was introduced.

[3] It should at least be documented (I didn't find anything, perhaps I was not searching in the right areas) It seems to be OK for me if ETL_NO_CHECKS is the documented way to do it and therefor I can make it dependent on NDEBUG which is the way I might use it. Others might have different requirements and because of that it might not make sense to make it dependent on NDEBUG by default (as it was in the past). I have to think more about it...

[4] No it's not OK They behavior shouldn't change if I use an ETL_ASSERT*. On reason is perhaps that someone uses an etl::error_handler to just log stuff and expects the code to run as if there is no logging. The etl::error_handler might just "flag" a problem and if it can decide that it is not "fatal" to go on. Perhaps someone can elaborate on the topic and the reason why this was changed.

Thanks for reading up to this line ;-) Please discuss in the comments...

[jwellbelove]

I added the ETL_ASSERT_OR_RETURN_VALUE and related macros so that there was an option for the application to continue or terminate after logging the error in the error handler.

You are correct in saying that the objects should be left in a logically valid state in this scenario. If they are not, then this is a bug. I try to minimise 'undefined behaviour' in the ETL.

I haven't given the 'legacy' classes much attention for a while.

[jwellbelove]

ETL_NO_CHECKS overrides every other error handler directive. If defined then every ETL_ASSERT_xxx will be a NOP. ETL_NO_CHECKS is for the scenario where the coder is looking to extract the absolute maximum performance. Not many people will want to choose this option, but it's there to use if they require it.

I've been working on a patch, with unit tests, to ensure that the legacy etl::bitset works in the same way as the new etl::bitset.

[jwellbelove]

Fixed 20.38.11