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

Fixed bugs in stack.c #61

Merged
merged 3 commits into from
Apr 1, 2024
Merged

Fixed bugs in stack.c #61

merged 3 commits into from
Apr 1, 2024

Conversation

sadeem-albir
Copy link
Contributor

Removed the else-block of "if (c == '-')" and restructured the clear command so the "stack is empty" message is not displayed an extra time after the c command has already been called. E.g if on the first line you type 'c', the message is displayed. On the next line, the message is displayed again even if you only typed "9 9+" because the previous code structure in the clear function forced the sp variable to go down to -1.

Removed the else-block of "if (c == '-')" and restructured the clear command so the "stack is empty" message is not displayed an extra time after the c command has already been called. E.g if on the first line you type 'c', the message is displayed. On the next line, the message is displayed again even if you only typed "9 9+" because the previous code structure in the clear function forced the sp variable to go down to -1.
Comment on lines 161 to 166
/* do
{
stack[sp] = 0.0;
} while (sp--);
*/
// Replacing the above commented code with this one ensures messages of "stack empty" aren't displayed an extra time after c command is called in the program
Copy link
Owner

Choose a reason for hiding this comment

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

@sadeem-albir Remove the commented code before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commented code has been removed.

modified the clear function
Removed else-block in if (c == '-')
Comment on lines +161 to +164
while (sp > 0)
{
stack[sp] = 0.0;
} while (sp--);
stack[--sp] = 0.0;
}
Copy link
Owner

Choose a reason for hiding this comment

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

@sadeem-albir Is this code required to change? What's the bug here? Is it that pre decrement that useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ohkimur If my assumption is correct, the sp works the same as buf and bufp. whenever you add something to the stack, you do stack[sp++], meaning the current index (zero) will be assigned then sp will be the next unassigned index. So by using that other while-loop instead, you ensure you move to the previous assigned sp until you reach zero. Also, in the do-loop, even if sp was zero, it will be decremented, undesirably becoming -1.

Copy link
Owner

Choose a reason for hiding this comment

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

@sadeem-albir With this approach, I think the first location from sp will be always empty. Can you verify that? What do you think would be better here? If the previous approach is better, I propose we go with 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.

@ohkimur Not necessarily a negative impact. As you see in the pop function, you return stack[--sp] so sp is meant to be empty at whatever current index it is lying at for the moment, and when we need to use a value from the stack, we simply call the previous index of sp, so if the stack was [1, 2, 3, 0] and sp is 3, calling pop will return the value of 3 at index 2. The change I have proposed ensures consistency with the way sp changes in the other stack functions, but if you believe staying at the previous change is better for the program, then the change doesn't have to be merged.

@ohkimur ohkimur merged commit a54e787 into ohkimur:main Apr 1, 2024
1 check passed
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