-
Notifications
You must be signed in to change notification settings - Fork 540
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
Fix switch-case condition error in sample code #245
Conversation
@@ -46,7 +46,8 @@ static int test_skcipher_result(struct skcipher_def *sk, int rc) | |||
switch (rc) { | |||
case 0: | |||
break; | |||
case -EINPROGRESS || -EBUSY: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read https://cbea.ms/git-commit/ carefully and enforce the rules to your git commit message.
The code `case -EINPROGRESS || -EBUSY: ` is the same as `case -115 || -16 :` at compiler time, as both error code are implemented with macro like `#define EBUSY 16`. The code above is essentially the same as `case 1:`. In C, there is no real boolean value. Boolean-like value will be converted to 1 or 0. It does not matter too much if the `-EINPROGRESS || -EBUSY` is calculated at build time or at runtime. In both case, it will compare the `rc` with `1` in the switch expression. It will not compare the `rc` with any real error code number. When the code is really `-EBUSY`, the execution will fallback to the default branch. And in practice, most of the compilers will do this simple compile-time static calculation, and generate code like ``` static int test_skcipher_result(struct skcipher_def *sk, int rc) { switch (rc) { case 0: break; case 1: rc = wait_for_completion_interruptible(&sk->result.completion); /* code removed for conciseness */ break; default: pr_info("skcipher encrypt returned with %d result %d\n", rc, sk->result.err); break; } init_completion(&sk->result.completion); return rc; } ```
Thanks for the link. I have rewritten the commit message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message looks good to me.
However, the code block with the ``` wrapper might be unnecessary.
Thank @srayuws for contributing! |
The code
case -EINPROGRESS || -EBUSY:
is the same ascase -115 || -16 :
at compiler time, as both error code are implemented with macro like#define EBUSY 16
.The code above is essentially the same as
case 1:
. In C, there is no real boolean value. Bool-like value will be converted to 1 or 0.It does not matter too much if the
-EINPROGRESS || -EBUSY
is calculated at build time or at runtime. In both case, it will compare therc
with1
in the switch expression. It will not compare therc
with any real error code number. When the code is really-EBUSY
, the execution will fallback to the default branch.And in practice, most of the compilers will do this simple compile-time static calculation, and generate code like