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

Fix switch-case condition error in sample code #245

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

srayuws
Copy link
Contributor

@srayuws srayuws commented Jan 1, 2024

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. 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 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;
}

@jserv jserv requested a review from linD026 January 1, 2024 06:38
@@ -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

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.

@srayuws srayuws changed the title condition error in switch-case Fix sample code error: condition error in switch-case Jan 1, 2024
@srayuws srayuws changed the title Fix sample code error: condition error in switch-case Fix sample code: condition error in switch-case Jan 1, 2024
Copy link
Contributor

@jserv jserv left a 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.

@srayuws srayuws changed the title Fix sample code: condition error in switch-case Fix switch-case condition error in sample code Jan 1, 2024
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;
}

```
@srayuws
Copy link
Contributor Author

srayuws commented Jan 1, 2024

Read https://cbea.ms/git-commit/ carefully and enforce the rules to your git commit message.

Thanks for the link. I have rewritten the commit message.

Copy link
Collaborator

@linD026 linD026 left a 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.

@jserv jserv merged commit 470fbcd into sysprog21:master Jan 3, 2024
1 check passed
@jserv
Copy link
Contributor

jserv commented Jan 3, 2024

Thank @srayuws for contributing!

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