[PATCH] D81272: [clang-tidy] New check `bugprone-redundant-branch-condition`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D81272#2218173 <https://reviews.llvm.org/D81272#2218173>, @whisperity wrote:

> In D81272#2218050 <https://reviews.llvm.org/D81272#2218050>, @aaron.ballman wrote:
>
>> Thanks to the new info, I think the check basically LGTM. Can you add some negative tests and documentation wording to make it clear that the check doesn't currently handle all logically equivalent predicates, like:
>>
>>   if (foo) {
>>   } else {
>>     if (!foo) {
>>     }
>>   }
>>   
>>   // or
>>   if (foo > 5) {
>>     if (foo > 3) {
>>     }
>>   }
>>   
>>   // or
>>   if (foo > 5) {
>>     if (5 < foo) {
>>     }
>>   }
>>
>> (I'm assuming these cases aren't handled currently and that handling them isn't necessary to land the patch.)
>
> I'm afraid handling such cases will eventually invoke the same problems the RangeConstraintSolver has, and then the discussion about whether this is good in Tidy instead of CSA will be resurrected... 😦
>
> One could come up with even more elaborate cases. Just hit something like below a few minutes ago while working:
>
>   const CallExpr* const C;
>   
>   if (auto* CalledFun = dyn_cast_or_null<FunctionDecl>(C->getCalleeDecl())) {
>     // ...
>     if (const auto* Fun = dyn_cast_or_null<FunctionDecl>(C->getCalleeDecl())) {
>       // ...
>     }
>     // ...
>   }
>
> and the rabbit hole goes deeper. But it's pretty interesting how many cases the current check found as-is.

While I agree with your observation about data and flow sensitivity, I approved on the belief that the check as-is provides enough utility to warrant adding it as-is. If someone wants to improve the check into being a CSA check, we can always deprecate this one at that point. However, if there are strong opinions that the check should start out as a CSA check because it requires that sensitivity for your needs, now's a good time to bring up those concerns.


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

https://reviews.llvm.org/D81272