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

Fix switch-case condition error in sample code #245

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Fix switch-case condition error in sample code #245

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion examples/cryptosk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ static int test_skcipher_result(struct skcipher_def *sk, int rc)
switch (rc) {
case 0:
break;
case -EINPROGRESS || -EBUSY:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, yes, this might be wrong since the case here should be an integer constant expression.

However, you should improve your commit message, for example, which compiler you used, the compiler error (warning) messages like:

warning: use of logical '||' with constant operand [-Wconstant-logical-operand]
...

, and why the logical operator (i.e., -EINPROGRESS || -EBUSY) is not the integer constant expression.

Even though, the standard doesn't explicitly tell us that the logical operator is not the integer constant expression, and the GCC also doesn't yell about this.

An integer constant expression shall have integer type and shall only have operands
that are integer constants, enumeration constants, character constants, sizeof
expressions whose results are integer constants, and floating constants that are the
immediate operands of casts. Cast operators in an integer constant expression shall only
convert arithmetic types to integer types, except as part of an operand to the sizeof
operator.

From ISO/IEC 9899:201x, 6.6 Constant expressions

The confusing point here is whether the evaluation of the logical operator happens at compile-time or runtime (it might be the implementation-defined). However, you need to explain to us why you want to change this properly.

I have no opposition to changing this code, but still, we need a good explanation to tell the following people why we are making this change.

Here are some links that might help you to write the message (you can even ask the ChatGPT to give suggestions, I think it is a good tool to explain the details of Standard), but you should see the Standard too.

https://en.cppreference.com/w/c/language/switch
https://en.cppreference.com/w/c/language/constant_expression

Copy link
Contributor Author

@srayuws srayuws Jan 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the long comment. I thought this is a straightforward one-line change and would not need a long description.
But yes, I can update my PR description later.

And to your question.
I am not sure if the compiler-time/runtime calculation is within some spec or just another compiler's UB/IB.
But the existing code is definitely wrong, no mater if it is a compile time calculation or not. Code like x==(a||b) will need to calculate the (a||b) first and generate a bool-like value, which is 1 or 0 in C. There is almost no chance to be correct, unless x is 1 and one of the a and b is also 1.

For a compiler, it is quite reasonable to perform the compile time static calculation. Both the error code are defined with #define E_some 100. To the compiler, the code is just like case -100 || -200 : at build time.

I tried some different toll combination on godbolt. All of them are showing the same result: static calculation at compile time and the case-condition is to compare with 1.

godbolt link: https://godbolt.org/z/1rPrPdjoh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with PR description, please let me know if there is any other ambiguity in it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok. Thanks for more comprehensive explanations.
Now, we should add PR description to the commit message too. So people can see these information within the git source tree.

You can use the git amend and force push.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

git commit updated as well.

case -EINPROGRESS:
case -EBUSY:
rc = wait_for_completion_interruptible(&sk->result.completion);
if (!rc && !sk->result.err) {
reinit_completion(&sk->result.completion);
Expand Down