ETLCPP / etl

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

Use constexpr for basic_string #964

Open ebmmy opened 1 month ago

ebmmy commented 1 month ago

This pull request intends to align the use of constexpr with how it is defined for `std::basic_string' in the STL.

jwellbelove commented 1 month ago

I'm pretty sure that this should work for C++14 constexpr, unless you know of any C++17/20 features that are required.

Also, you have not created any unit tests to check that all of the constexpr functions can be used in a constexpr expression.

ebmmy commented 1 month ago

I'm pretty sure that this should work for C++14 constexpr, unless you know of any C++17/20 features that are required.

Right, ETL_CONSTEXPR14 should work fine, I was not sure whether your policy was to conform to the C++ standard or use the first possible version for constexpr.

Also, you have not created any unit tests to check that all of the constexpr functions can be used in a constexpr expression.

What would be the purpose of such tests? This wouldn't really test a functionality, either the function isn't marked as constexpr', the expression wouldn't compile, or it is marked asconstexpr' and it can be used inside a `constexpr' expression. What do you think?

jwellbelove commented 1 month ago

The purpose of the constexpr tests is to make sure that the compiler is happy that a function marked as constexpr can actually work in a constexpr expression.

ebmmy commented 1 month ago

The purpose of the constexpr tests is to make sure that the compiler is happy that a function marked as constexpr can actually work in a constexpr expression.

Isn’t that granted by the c++ standard directly? I cannot really think of a case this isn’t true.

jwellbelove commented 1 month ago

Certain compilers will only analyse the template code for what you actually invoke. For example, Microsoft's compiler will not alert that a template function cannot be used in a constexpr unless you actually create code that uses it in a constexpr.

Example:

template <typename T>
constexpr int GetValue(T i)
{
  static int si = 2;

  return i + si;
}

int main()
{
  constexpr int x = GetValue<int>(1); // This is a compile error for C++14 with VS2022

  int y = GetValue<int>(1);           // This is not a compile error for C++14 with VS2022

  return 0;
}
jwellbelove commented 3 weeks ago

What about the constexpr support for the top level classes etl::string, etl::wstring, etl::u8string, etl::u16string, u32string?

jwellbelove commented 3 weeks ago

The unit tests fail for C++11.

ebmmy commented 2 days ago

Hello! Thanks a lot for the explanation about VS compiler, I'm never using it. Also sorry for the long delay I was away.

What about the constexpr support for the top level classes etl::string, etl::wstring, etl::u8string, etl::u16string, u32string?

I think I need to have a deeper look for this, becquse the ibasic_string type is not litteral hence we cannot declare a constexpr variable for those derived classes. I'll try to think on how to rework the PR for this.

jwellbelove commented 1 day ago

I had issues trying to make etl::bitset constexpr. In the end, I had to discard using etl::ibitset and pass the buffer with every top-level member function to a private implementation class. I don't want to have to do that with the general ETL containers.