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 a bitwise instead of logical warning #2746

Closed
wants to merge 1 commit into from

Conversation

spinicist
Copy link
Contributor

There's a missing | in notcurses.h that gives:

warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]

This commit adds said |

@dankamongmen
Copy link
Owner

NAK. see the comment above the code:

  // bitwise or is intentional (allows compiler more freedom)

what you/we want to do is add the parens.

@dankamongmen
Copy link
Owner

but thank you for calling attention to this!

@dankamongmen
Copy link
Owner

hrmm, adding the parens triggers a new warning -Wbitwise-instead-of-logical

@dankamongmen
Copy link
Owner

@dankamongmen
Copy link
Owner

alright, i think the way we solve this is break down the two calls into a single check, which we can do (and the compiler can presumably also do, so this is kinda unfortunate, as it's a hit to readability and modularity).

we can't just locally turn this bogon IMHO warning off, since this is a public header. lame.

@dankamongmen
Copy link
Owner

return !((ncchannel_default_p(channel) | ncchannel_palindex_p(channel)));

=

return !(
ncchannel_default_p(channel)
|
(!ncchannel_default_p(channel) && (channel & NC_BG_PALETTE))
)

=

return !(
if(ncchannel_default_p(channel)){
  return true;
}else if(channel & NC_BG_PALETTE){
  return true;
}
}
)

right?

@dankamongmen
Copy link
Owner

but that still has the decision point present -- it's no improvement, compiler freedom wise, over what we already have. and it's a hit to readability.

so maybe we just roll with @spinicist 's patch, plus removal of the comment above.

we'd also want this on internal.h afaict

@dankamongmen
Copy link
Owner

alright, if you're around at the moment, just kill the comment above your change, and i'll merge. otherwise i'll just do this myself. i guess if i merge your branch locally it'll retain your commit? let me try that.

@dankamongmen
Copy link
Owner

ok, i've cherry-picked d9cc551 and pushed it. that preserved credit for the change (though not github points, alas). thanks!

@dankamongmen dankamongmen self-requested a review January 10, 2024 07:15
@spinicist
Copy link
Contributor Author

I'm in a different timezone, no problem about the Github points. Sorry I didn't read the comment on the line above. Thanks for merging!

@spinicist spinicist deleted the bitwise branch January 10, 2024 10:12
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