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.

Sign up for GitHub

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

Implement Constexpr Strong Typedef #861

Merged
merged 7 commits into from
Mar 13, 2024
Merged

Implement Constexpr Strong Typedef #861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
adb50a6
feat: make type_def constexpr
drewr95 Mar 12, 2024
0617696
test: macro constexpr
drewr95 Mar 12, 2024
089c122
test: implicit constexpr
drewr95 Mar 12, 2024
f1d4ef9
test: get constexpr
drewr95 Mar 12, 2024
35d2226
refactor: remove constexpr from assignment
drewr95 Mar 12, 2024
014dc5d
test: comparisons constexpr
drewr95 Mar 12, 2024
11c92d5
fix: cpp11 support for constexpr get method
drewr95 Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 21 additions & 13 deletions include/etl/type_def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,25 +58,25 @@ namespace etl
typedef TIdType id_type;

//*********************************************************************
type_def()
ETL_CONSTEXPR type_def()
: value(TValue())
{
}

//*********************************************************************
type_def(TValue value_)
ETL_CONSTEXPR type_def(TValue value_)
: value(value_)
{
}

//*********************************************************************
type_def(const type_def& other)
ETL_CONSTEXPR type_def(const type_def& other)
: value(other.value)
{
}

//*********************************************************************
operator TValue() const
ETL_CONSTEXPR operator TValue() const
{
return value;
}
Expand Down Expand Up @@ -238,63 +238,71 @@ namespace etl
}

//*********************************************************************
type_def& operator =(TValue rhs)
ETL_CONSTEXPR type_def& operator =(TValue rhs)
{
value = rhs;
return *this;
}

//*********************************************************************
type_def& operator =(const type_def& rhs)
ETL_CONSTEXPR type_def& operator =(const type_def& rhs)
{
value = rhs.value;
return *this;
}

//*********************************************************************
TValue& get()
ETL_CONSTEXPR TValue& get()
Copy link
Contributor Author

@drewr95 drewr95 Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jwellbelove I believe I may have mistaken this part. I think for C++11 users this will make the TValue reference be const? In C++14 they loosened the restrictions on what constexpr meant, so methods didn't have to be necessarily constexpr to have the keyword for it. I can change this quickly if you suspect this is the case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is normal to not have constexpr on both the const and non-const functions.
I'm pretty sure you don't need the #if ETL_USING_CPP14, unless I'm missing something.
I'm also sure that the operators can also be constexpr.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you like? I thought for CPP11 constexpr was strict? I can leave the constexpr off the get methods. As far as operators, I left the assignment operators (*=, +=, -=, ++, --, etc) not constexpr because there wasn't a way for me to test them. In C++14 it would have been okay to make them constexpr because of the more lax rules. But I'm pretty sure that would have failed the C++11 actions. I think i'll just leave the gets as non-constexpr and open a new PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found I get a compile error if I've set both const and non-const versions of a member function constexpr.

The operators can be tested by making a unit test that is only enabled for C++14 and above.
The CI that I run locally, and on Github Actions, test with C++11, C++14, C++17 & C++20

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update the tests for C++14 and above, thanks.

Copy link
Contributor Author

@drewr95 drewr95 Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure how they can be constexpr without specifying #if ETL_USING_CPP14, can you clarify? It's not making sense how the C++11 builds would pass since all the operator *= += -= tests will be non-constexpr. As you pointed out, I would have to create separate tests for C++14 and above - which is "ETL_USING_CPP14" if I'm not mistaken. Why would the tests need differentiated but not the implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of me thinks the assignment operators shouldn't be marked constexpr regardless because you can't manipulate a constexpr variable. So I almost feel like the PR should stay the way it is (I removed the constexpr keywords from the get and assignment operators).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can able to use constexpr operators inside a constexpr function that returns a constexpr object.

Example
https://godbolt.org/z/rPqW17xPx

There is more than one constexpr macro.
ETL_CONSTEXPR or ETL_CONSTEXPR11 Defined as constexpr for C++11 and above
ETL_CONSTEXPR14 Defined as constexpr for C++14 and above
ETL_CONSTEXPR17 Defined as constexpr for C++17 and above
ETL_CONSTEXPR20 Defined as constexpr for C++20 and above

You use the one that is appropriate for the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, much appreciated! I will get right on that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still getting build errors. I'm kind of stumped. The lambdas were nice to keep the typedefs local to the test functions. If I move the lambdas out to constexpr free functions like your godbolt-linked example, then the typedef will need to exist at a scope outside of the tests (which will conflict with the tests). Unless you're okay with that or have another suggestion

{
return value;
}

#if ETL_USING_CPP14
//*********************************************************************
ETL_CONSTEXPR const TValue& get() const
{
return value;
}
#else
//*********************************************************************
const TValue& get() const
{
return value;
}
#endif // ETL_USING_CPP14

//*********************************************************************
friend bool operator <(const type_def& lhs, const type_def& rhs)
friend ETL_CONSTEXPR bool operator <(const type_def& lhs, const type_def& rhs)
{
return lhs.value < rhs.value;
}

//*********************************************************************
friend bool operator <=(const type_def& lhs, const type_def& rhs)
friend ETL_CONSTEXPR bool operator <=(const type_def& lhs, const type_def& rhs)
{
return lhs.value <= rhs.value;
}

//*********************************************************************
friend bool operator >(const type_def& lhs, const type_def& rhs)
friend ETL_CONSTEXPR bool operator >(const type_def& lhs, const type_def& rhs)
{
return lhs.value > rhs.value;
}

//*********************************************************************
friend bool operator >=(const type_def& lhs, const type_def& rhs)
friend ETL_CONSTEXPR bool operator >=(const type_def& lhs, const type_def& rhs)
{
return lhs.value >= rhs.value;
}

//*********************************************************************
friend bool operator ==(const type_def& lhs, const type_def& rhs)
friend ETL_CONSTEXPR bool operator ==(const type_def& lhs, const type_def& rhs)
{
return lhs.value == rhs.value;
}

//*********************************************************************
friend bool operator !=(const type_def& lhs, const type_def& rhs)
friend ETL_CONSTEXPR bool operator !=(const type_def& lhs, const type_def& rhs)
{
return lhs.value != rhs.value;
}
Expand Down
71 changes: 71 additions & 0 deletions test/test_type_def.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,21 @@ namespace
CHECK_EQUAL(i1, i2);
}

//*************************************************************************
TEST(test_macro_constexpr)
{
ETL_TYPEDEF(uint32_t, type1_t);
ETL_TYPEDEF(uint32_t, type2_t);

constexpr type1_t t1 = type1_t(1);
constexpr type2_t t2 = type2_t(1);

uint32_t i1 = t1.get();
uint32_t i2 = t2.get();

CHECK_EQUAL(i1, i2);
}

//*************************************************************************
TEST(test_implicit)
{
Expand All @@ -69,6 +84,24 @@ namespace
CHECK_EQUAL(i1, i2);
}

//*************************************************************************
TEST(test_implicit_constexpr)
{
class type1_t_tag;
typedef etl::type_def<type1_t_tag, uint32_t> type1_t;

class type2_t_tag;
typedef etl::type_def<type2_t_tag, uint32_t> type2_t;

constexpr type1_t t1 = type1_t(1);
constexpr type2_t t2 = type2_t(1);

uint32_t i1 = t1.get();
uint32_t i2 = t2.get();

CHECK_EQUAL(i1, i2);
}

//*************************************************************************
TEST(test_get)
{
Expand All @@ -84,6 +117,21 @@ namespace
CHECK_EQUAL(t1.get(), t2.get());
}

//*************************************************************************
TEST(test_get_constexpr)
{
class type1_t_tag;
typedef etl::type_def<type1_t_tag, uint32_t> type1_t;

class type2_t_tag;
typedef etl::type_def<type2_t_tag, uint32_t> type2_t;

constexpr type1_t t1(1);
constexpr type2_t t2(1);

CHECK_EQUAL(t1.get(), t2.get());
}

//*************************************************************************
TEST(test_operators)
{
Expand Down Expand Up @@ -141,5 +189,28 @@ namespace
CHECK(!(t1 >= t2));
CHECK(t2 >= t4);
}

//*************************************************************************
TEST(test_comparisons_constexpr)
{
class __type_t__;
typedef etl::type_def<__type_t__, uint32_t> type_t;

constexpr type_t t1(1);
constexpr type_t t2(2);
constexpr type_t t3(t1);
constexpr type_t t4(t2);

CHECK(t1 < t2);
CHECK(!(t2 < t1));
CHECK(t1 <= t2);
CHECK(!(t2 <= t1));
CHECK(t1 <= t3);
CHECK(t2 > t1);
CHECK(!(t1 > t2));
CHECK(t2 >= t1);
CHECK(!(t1 >= t2));
CHECK(t2 >= t4);
}
};
}