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

test: Check the types of execution arguments #655

Merged
merged 8 commits into from
Dec 30, 2020
Merged

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Nov 20, 2020

After previously introducing TypedValue in #658, this PR adds type checking for execution arguments in unit tests. The type checking is done in fizzy::test::execute() so can be bypassed by using fizzy::execute() directly. Still most of the tests uses the former.

Later we will add type checking to the execution result and the Result() matcher. We will be also able to see the type-checking bypassing cases and correct these if needed.

@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #655 (9c42204) into master (f8f26c5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #655   +/-   ##
=======================================
  Coverage   99.27%   99.28%           
=======================================
  Files          71       72    +1     
  Lines        9855     9918   +63     
=======================================
+ Hits         9784     9847   +63     
  Misses         71       71           
Flag Coverage Δ
spectests 91.48% <ø> (ø)
unittests 99.28% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
test/unittests/api_test.cpp 100.00% <100.00%> (ø)
test/unittests/execute_control_test.cpp 99.46% <100.00%> (ø)
test/unittests/execute_floating_point_test.cpp 99.69% <100.00%> (ø)
test/unittests/execute_invalid_test.cpp 100.00% <100.00%> (ø)
test/unittests/execute_numeric_test.cpp 100.00% <100.00%> (ø)
test/unittests/execute_test.cpp 100.00% <100.00%> (ø)
test/unittests/typed_value_test.cpp 100.00% <100.00%> (ø)
test/utils/execute_helpers.hpp 100.00% <100.00%> (ø)
test/utils/typed_value.hpp 100.00% <100.00%> (ø)
... and 1 more

@chfast chfast force-pushed the test_typed_value branch 2 times, most recently from f924bd4 to 7b9631b Compare November 20, 2020 13:28
@chfast chfast force-pushed the test_typed_value branch 3 times, most recently from 4b29dd7 to 65f595f Compare November 21, 2020 16:18
@chfast chfast changed the base branch from master to typed_value November 21, 2020 16:29
@chfast chfast requested review from gumb0 and axic November 21, 2020 16:34
@chfast chfast force-pushed the test_typed_value branch 2 times, most recently from e637ff1 to 43de6ca Compare November 21, 2020 20:00
@chfast chfast marked this pull request as ready for review November 21, 2020 20:15
Base automatically changed from typed_value to master November 23, 2020 14:59
@chfast chfast changed the title test: Check the types of args to unary op tests test: Check the types of execution arguments Nov 23, 2020
@chfast chfast force-pushed the test_typed_value branch 2 times, most recently from acdec1b to 678af2f Compare November 23, 2020 19:15
test/unittests/api_test.cpp Outdated Show resolved Hide resolved
test/unittests/api_test.cpp Outdated Show resolved Hide resolved
test/unittests/api_test.cpp Outdated Show resolved Hide resolved
@chfast
Copy link
Collaborator Author

chfast commented Nov 25, 2020

I only changed the values required to compile and pass the test.

The 0 work when decltype(0) === int32_t. It will also compile (and work incorrectly) if decltype(0) === int64_t. Otherwise this will fail to compile.

This variant is "middle-ground" for convenience and type safety - it implements constructors for int32_t, uint32_t, int64_t, uint64_t. This requires to bump the value to u64 when i64 type is required.

The less strict variant is to conditionally at runtime zero extend the uint32_t and int32_t values to i64 when they are not negative (e.g. 1 and -1u are fine, but -1 is not). I'm not sure this is easy to implement here, but I did it for Result() in #659.

The more strict variant is to only implement constructors for uint32_t and uint64_t. This is what I tried first. It is more annoying. It also encourages things like -1_u32.

That's why I stayed with minimal changes to focus on design discussion rather than esthetics.

My personal opinions is that this variant is good enough and definitely good for stating point to experiment with other variants on top of it. I also want to consider the option to promote this API to public type safe API. I prefer having one common type-safe API.

@gumb0
Copy link
Collaborator

gumb0 commented Nov 25, 2020

The set of constructors looks good and logical. It shouldn't be less strict, because then tests wouldn't show what types are really expected. It shouldn't be more more strict, because signed->unsigned conversion is already handled by Value.

But I still don't get why it's better to rely on implicit conversion (which might theoretically be different for different platform).
And why is it not everywhere but only for second argument?
(I mean you do use _u32 when it's the single argument

EXPECT_THAT(execute(*instance_reordered, 1, {0_u32}), Result(1));
EXPECT_THAT(execute(*instance_reordered, 3, {0_u64, 0}), Result());

)

Is it just to minimize changes in this PR? What is you preference for the new tests that will execute functions with two parameters?

@chfast
Copy link
Collaborator Author

chfast commented Nov 25, 2020

The {0_u32} (previously {Value{0}}) handles other famous C++ issue where 0 binds to a pointer type instead of Value because the former is built-in conversion and later is user-provided conversion. Somehow the uint32_t does not. Except for this case, I have not introduced 0_u32 otherwise (at least not intentionally).

Keep in mind that _u32 only creates the uint32_t objects, not Values.

EXPECT_EQ(1_u64, uint64_t{1});
EXPECT_EQ(0xffffffff_u64, uint64_t{0xffffffff});
EXPECT_EQ(0x100000000_u64, uint64_t{0x100000000});
EXPECT_EQ(0xffffffffffffffff_u64, uint64_t{0xffffffffffffffff});
Copy link
Member

Choose a reason for hiding this comment

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

What happens to literals larger than 64-bit? Are 128-bit literals supported where __uint128_t is supported?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are no builtin integer literals bigger than unsigned long long. The __int128 does not have its own literal.

// >32-bits set
EXPECT_THAT(
execute_unary_operation(Instr::i32_wrap_i64, 0xffffffffffffffff), Result(0xffffffff));
execute_unary_operation(Instr::i32_wrap_i64, 0xffffffffffffffff_u64), Result(0xffffffff));
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, perhaps result could be also using the _u32/_u64 literals.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is separate issue being handled in #659.

@chfast chfast force-pushed the test_typed_value branch 2 times, most recently from 979b997 to b96047f Compare December 28, 2020 15:50
@chfast chfast merged commit 2ee7be6 into master Dec 30, 2020
@chfast chfast deleted the test_typed_value branch December 30, 2020 09:04
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