Page MenuHomePhabricator

[clang-tidy] false-positive for bugprone-redundant-branch-condition in case of passed-by-ref params

Authored by zinovy.nis on Nov 15 2020, 7:19 AM.

Diff Detail

Event Timeline

zinovy.nis created this revision.Nov 15 2020, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2020, 7:19 AM

Extend the context.

Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko added a project: Restricted Project.

Thank you for working on this!


Did you mean for the parameter to be a reference?


FWIW, I would expect this one to be diagnosed if the call really was byval instead of byref.


Another test that would be interesting is:

if (tryToExtinguish(isSet) && isSet) {
  if (tryToExtinguishByVal(isSet) && isSet) { // Dupe check of isSet should be diagnosed

if (tryToExtinguishByVal(isSet) && isSet) {
  if (tryToExtinguish(isSet) && isSet) { // Ok
zinovy.nis added inline comments.Nov 18 2020, 11:58 AM

Nice catch! Thanks. Looks like isChangedBefore is too rough and must be refined

Handle ref & val mixed cases.

zinovy.nis marked 3 inline comments as done.Nov 25 2020, 10:00 AM

What do you think of it?

aaron.ballman added inline comments.Dec 4 2020, 11:06 AM

I think this comment should be hoisted above to update the comment on line 32.


80-col limit issue -- you should run the patch through clang-format.


const auto *, but I think this would be better expressed as:

const DeclRefExpr *OuterIfVar, *InnerIfVar;
if (const auto *Outer = Result.Nodes.getNodeAs<DeclRefExpr>(OuterIfVar1Str))
  OuterIfVar = Outer;
  OuterIfVar = Result.Nodes.getNodeAs<DeclRefExpr>(OuterIfVar2Str);

// Similar for InnerIfVar
zinovy.nis updated this revision to Diff 309732.Dec 5 2020, 4:11 AM

Fixed remarks from Aaron.

zinovy.nis marked 3 inline comments as done.Dec 5 2020, 4:12 AM
aaron.ballman accepted this revision.Dec 10 2020, 10:01 AM

LGTM, thank you for the fix!

This revision is now accepted and ready to land.Dec 10 2020, 10:01 AM