dankamongmen / notcurses

blingful character graphics/TUI library. definitely not curses.
https://nick-black.com/dankwiki/index.php/Notcurses
Other
3.57k stars 112 forks source link

Fix a bitwise instead of logical warning #2746

Closed spinicist closed 8 months ago

spinicist commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

but thank you for calling attention to this!

dankamongmen commented 8 months ago

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

dankamongmen commented 8 months ago

hahahah look at this https://stackoverflow.com/questions/75911803/error-use-of-bitwise-with-boolean-operands

dankamongmen commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

spinicist commented 8 months ago

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!