ETLCPP / etl

Embedded Template Library
https://www.etlcpp.com
MIT License
2.05k stars 372 forks source link

new etl::bitset does not compile for 32bit compilation #808

Closed TG-SAG closed 2 months ago

TG-SAG commented 6 months ago

I'm currently switching our project from the legacy etl::bitset<> to the new etl::bitset<>. But I have problems compiling the unit tests for a 32bit platform (we explicitly instantiate a selection of sizes to make sure they will compile): https://gcc.godbolt.org/z/Yrh7G9rMd

template class etl::bitset<64>;

The above line will produce the follwing error:

/opt/compiler-explorer/libs/etl/include/etl/private/bitset_new.h: In instantiation of 'constexpr typename etl::enable_if<etl::is_integral<TIterator>::value, T>::type etl::bitset<Active_Bits, TElement, false>::value() const [with T = long unsigned int; unsigned int Active_Bits = 64; TElement = char; typename etl::enable_if<etl::is_integral<TIterator>::value, T>::type = long unsigned int]':
/opt/compiler-explorer/libs/etl/include/etl/private/bitset_new.h:2035:34:   required from 'constexpr long unsigned int etl::bitset<Active_Bits, TElement, false>::to_ulong() const [with unsigned int Active_Bits = 64; TElement = char]'
<source>:4:21:   required from here
/opt/compiler-explorer/libs/etl/include/etl/private/bitset_new.h:2025:7: error: static assertion failed: Type too small
 2025 |       ETL_STATIC_ASSERT((sizeof(T) * CHAR_BIT) >= (Number_Of_Elements * Bits_Per_Element), "Type too small");
      |       ^~~~~~~~~~~~~~~~~
/opt/compiler-explorer/libs/etl/include/etl/private/bitset_new.h:2025:7: note: the comparison reduces to '(32 >= 64)'

I will seems to work for a 64bit compilation: https://gcc.godbolt.org/z/h5T87Eof1

Before I start fixing the problem I wanted to ask if someone already has a fix for this (because it shouldn't be that uncommon to have a 32bit compilation)

jwellbelove commented 6 months ago

I'm not sure why the template function value() is being instantiated when the compiler has absolutely no idea what the requested return type actually might be!

Where exactly is T being defined in the declaration template class etl::bitset<64>; for the template member function below?

Why is it assuming T = long unsigned int; when value() is never invoked?

template <typename T>
ETL_CONSTEXPR14
typename etl::enable_if<etl::is_integral<T>::value, T>::type
  value() const ETL_NOEXCEPT
{
  ETL_STATIC_ASSERT(etl::is_integral<T>::value, "Only integral types are supported");
  ETL_STATIC_ASSERT((sizeof(T) * CHAR_BIT) >= (Number_Of_Elements * Bits_Per_Element), "Type too small");

  return ibitset.template value<T>(buffer, Number_Of_Elements);
}
TG-SAG commented 6 months ago

The to_ulong() and to_ullong() methods use value<>() with unsigned long and unsigned long long respectively. The ETL implementation always does a compile time check if these methods can be used based on the active bits while the STL does a runtime check with exceptions (Playground: https://gcc.godbolt.org/z/rj9o6b3T5) I would expect this behavior to be intentional because of (possibly) missing C++ exceptions in an embedded environment (isn't it?). I might not have the need to explicitly instantiate the etl::bitset<> to test if everything compiles and just delete it. I suppose this could be fixed by "enableif'ing" to_ulong() and to_ullong() dependend on the number of active bits but currently I don't think it's worth the effort.

jwellbelove commented 6 months ago

Ah yes, I see. The error only mentioned value(). I forgot about those other member functions. Adding enable_if would be pretty simple to add; I may add it while I'm doing the current changes.

As well as the requested extract functions I'm currently working on, I'm looking at reducing the amount of code generated for varying bit lengths that fit into a single integer.

For example, etl::bitset<5, uint8_t>, etl::bitset<7, uint8_t> & etl::bitset<8, uint8_t> would cause three almost identical classes to be generated. I am looking at moving a lot of the guts of the top level classes into a bit length independent implementation.

jwellbelove commented 6 months ago

Actually, a static_assert would be easier than enable_if. (Though it wouldn't solve your problem)

Also, to address the code size issues, I'm thinking of mandating that the element type must be unsigned.

jwellbelove commented 2 months ago

Fixed 20.38.11