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

capi: Add static assertions for FizzyValueType* vs. fizzy:ValType::* #821

Merged
merged 3 commits into from
May 24, 2022

Conversation

axic
Copy link
Member

@axic axic commented May 23, 2022

No description provided.

@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #821 (8482896) into master (8482896) will not change coverage.
The diff coverage is n/a.

❗ Current head 8482896 differs from pull request most recent head 6309ebe. Consider uploading reports for the commit 6309ebe to get more accurate results

@@           Coverage Diff           @@
##           master     #821   +/-   ##
=======================================
  Coverage   99.28%   99.28%           
=======================================
  Files          86       86           
  Lines       13221    13221           
=======================================
  Hits        13126    13126           
  Misses         95       95           
Flag Coverage Δ
rust 98.67% <0.00%> (ø)
spectests 89.98% <0.00%> (ø)
unittests 99.21% <0.00%> (ø)
unittests-32 99.31% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

lib/fizzy/cxx23/utility.hpp Outdated Show resolved Hide resolved
{
A = 0xff
};
EXPECT_EQ(to_underlying(A::A), 0xff);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add `static_assert(std::is_same_v<decltype(to_underlying(A::A)), int>);

Copy link
Member Author

Choose a reason for hiding this comment

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

Specs said it is at least int. So this may fail on some platforms?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also did you know that the C enum (decltype(C::A)) defaults to unsigned int?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It will be bigger than int only if some values don't fit int

Copy link
Collaborator

Choose a reason for hiding this comment

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

For scoped enumerations (enum class) the implicit underlying type is int. In case value do not fit, this is compile error. https://godbolt.org/z/1x8ffaadW.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like we are adding C++ compiler compliance tests at this point 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just wanted to know if this returns the same type as in the enum declaration. If you want to be explicit, you can define it as enum class A : int.

{
A = 0xff
};
static_assert(std::is_same_v<decltype(to_underlying(C::A)), unsigned int>);
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this stable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. This can be any integer type.

@axic axic force-pushed the capi-valtype branch 2 times, most recently from 625c375 to a229fe1 Compare May 24, 2022 13:33
static_assert(FizzyValueTypeF64 == fizzy::to_underlying(fizzy::ValType::f64));
static_assert(FizzyValueTypeVoid == 0);
static_assert(std::is_same_v<decltype(fizzy::to_underlying(fizzy::ValType::i32)),
std::remove_const<decltype(FizzyValueTypeI32)>::type>);
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this overkill?

{
A = 0xff
};
static_assert(std::is_same_v<decltype(to_underlying(C::A)), unsigned int>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

No. This can be any integer type.

{
A = 0xff
};
EXPECT_EQ(to_underlying(A::A), 0xff);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just wanted to know if this returns the same type as in the enum declaration. If you want to be explicit, you can define it as enum class A : int.

@axic
Copy link
Member Author

axic commented May 24, 2022

I just wanted to know if this returns the same type as in the enum declaration. If you want to be explicit, you can define it as enum class A : int.

The others are explicit, intentionally left in the implicit one.

@axic axic merged commit 3b5ee4e into master May 24, 2022
@axic axic deleted the capi-valtype branch May 24, 2022 21:31
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.

3 participants