[PATCH] D108003: [Clang] Extend -Wbool-operation to warn about bitwise and of bools with side effects

[Ryan Beltran via Phabricator via cfe-commits]

rpbeltran added a comment.

Sorry for the late reply on this. I ran the warning against ChromeOS and 
actually found very few false positives and a couple interesting findings to 
file bugs for. I actually saw about an equal number of cases caught for `&` and 
`|`.

https://source.chromium.org/chromium/chromium/src/+/main:third_party/perfetto/src/perfetto_cmd/perfetto_cmd.cc;l=492?q=%22(is_attach()%20%7C%20is_detach()%20%7C%7C%20query_service_%20%7C%7C%20has_config_options))%22

https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v4.4/drivers/gpu/drm/i915/i915_drv.h?q=%22i915_reset_in_progress(error)%20%7C%20i915_terminally_wedged(error)%22

https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/power_manager/powerd/policy/charge_controller.cc?q=%22ApplyPeakShiftChange(policy)%20%26%20ApplyBootOnAcChange(policy)%20%26%22

https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/src/libANGLE/renderer/gl/VertexArrayGL.cpp?q=%22const%20bool%20enabled%20%3D%20mState.getVertexAttribute(attribIndex).enabled%20%26%22

https://source.chromium.org/chromium/chromium/src/+/main:third_party/harfbuzz-ng/src/src/hb-ot-layout-gpos-table.hh?q=%22if%20(valueFormats%5B0%5D.apply_value%20(c,%20this,%20%26record-%3Evalues%5B0%5D,%20buffer-%3Ecur_pos())%20%7C%22

https://source.chromium.org/chromium/chromium/src/+/main:third_party/harfbuzz-ng/src/src/hb-ot-layout-gpos-table.hh?q=%22if%20(valueFormat1.apply_value%20(c,%20this,%20v,%20buffer-%3Ecur_pos())%20%7C%22


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108003/new/

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108003: [Clang] Extend -Wbool-operation to warn about bitwise and of bools with side effects

[Ryan Beltran via Phabricator via cfe-commits]

rpbeltran added a comment.

In D108003#2956332 , @xbolva00 wrote:

>>> and it would be more of an optimization than correctness issue as far as I 
>>> understand
>
> Yeah, this is right, indeed.
>
> Maybe @rpbeltran has some idea or motivating cases for OR pattern?

I don't actually have a case off-hand that I've spotted from the real world, 
but this example snippet seems plausible enough:

  if (queue.empty() || queue.pop() == STOP_TOKEN) {
break;
  }

Consider that in this scenario, we only consider queue.pop() if the queue is 
not empty due to short circuiting.

  if (queue.empty() | queue.pop() == STOP_TOKEN) {
break;
  }

While this scenario calls pop() regardless, likely causing a runtime error if 
the queue is empty.

I'll run this warning against ChromeOS and see if I spot any interesting 
results.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108003/new/

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108003: [Clang] Extend -Wbool-operation to warn about bitwise and of bools with side effects

[Ryan Beltran via Phabricator via cfe-commits]

rpbeltran added a comment.

Thanks! And sorry for missing that point in your first comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108003/new/

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108003: [Clang] Extend -Wbool-operation to warn about bitwise and of bools with side effects

[Ryan Beltran via Phabricator via cfe-commits]

rpbeltran added a comment.

This patch seems like a great contribution! Really glad to see this being 
added. I did have a question though on why this only appears to catch "&" vs 
"&&" instead of doing the same for "|" vs "||". It seems like both operators 
have roughly the same potential for confusion. Could we add support for bitwise 
vs logical or in this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108003/new/

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits