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

Adjust OperandStack allocation size when no locals #630

Merged
merged 2 commits into from
Jan 21, 2021
Merged

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented Nov 2, 2020

Fixes #583.

@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #630 (4c9677f) into master (15970f1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #630   +/-   ##
=======================================
  Coverage   99.30%   99.30%           
=======================================
  Files          72       72           
  Lines       10300    10312   +12     
=======================================
+ Hits        10228    10240   +12     
  Misses         72       72           
Flag Coverage Δ
spectests 91.50% <100.00%> (+<0.01%) ⬆️
unittests 99.30% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
lib/fizzy/stack.hpp 100.00% <100.00%> (ø)
test/unittests/stack_test.cpp 100.00% <100.00%> (ø)


// Even when stack is empty, there exists a single hidden item slot.
const_cast<fizzy::Value*>(stack.rbegin())->i64 = 1;
EXPECT_EQ(stack.rbegin()->i64, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I think we should document this behaviour in the header.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. This is a logical error to do this.

Copy link
Member

Choose a reason for hiding this comment

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

The documentation (https://en.cppreference.com/w/cpp/iterator/begin) doesn't state anywhere that begin() would be UB in case of size() == 0.

Copy link
Member

Choose a reason for hiding this comment

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

@chfast any feedback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Taking the iterator is fine, but you cannot dereference it when it is the end iterator.

Copy link
Member

Choose a reason for hiding this comment

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

But here you are dereferencing it?

To be honest at this stage I lost what the conversation is about.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay, we do ensure that begin() == end(), just that we know begin() is always valid.

@chfast chfast requested a review from gumb0 December 30, 2020 17:12
@axic
Copy link
Member

axic commented Jan 20, 2021

@chfast can this be merged?

@chfast
Copy link
Collaborator Author

chfast commented Jan 21, 2021

If you really want to.

@chfast chfast merged commit c46166b into master Jan 21, 2021
@chfast chfast deleted the operand_stack branch January 21, 2021 11:44
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.

UB in OperandStack
3 participants