Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use constexpr for basic_string #964

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ebmmy
Copy link

@ebmmy ebmmy commented Oct 1, 2024

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

@jwellbelove
Copy link
Contributor

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
Copy link
Author

ebmmy commented Oct 9, 2024

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 as constexpr' and it can be used inside a `constexpr' expression. What do you think?

@jwellbelove
Copy link
Contributor

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
Copy link
Author

ebmmy commented Oct 9, 2024

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
Copy link
Contributor

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
Copy link
Contributor

jwellbelove commented Oct 22, 2024

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

@jwellbelove
Copy link
Contributor

The unit tests fail for C++11.

@ebmmy
Copy link
Author

ebmmy commented Nov 17, 2024

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants