[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54757#1311516 <https://reviews.llvm.org/D54757#1311516>, @Szelethus wrote:

> In D54757#1311468 <https://reviews.llvm.org/D54757#1311468>, @donat.nagy wrote:
>
> > **Macros:**
> >
> > The current implementation of the check only looks at the preprocessed code, therefore it detects the situations where the duplication is created by macros. I added some tests to highlight this behavior.
>
>
> The only option I see to detect macro related errors is to use a `Lexer`, and compare the tokens one by one, but that might be overkill, and I'm aware of a check in the works that already implements that. I think it's perfectly fine not to cover those cases, but you should definitely document it somewhere, preferably in the rst file.


Agreed with the documentation note.
Starting around with the Lexer here will not help, it is too fragile, as macros can change between different builds and so on.

>> I think that in some situations this would be useful for detecting redundant or incorrect parts of macro-infected code.
> 
> How about this?
> 
>   #define PANDA_CUTE_FACTOR 9001
>   #define CAT_CUTE_FACTOR 9001
>   
>   if (whatever())
>     return PANDA_CUTE_FACTOR;
>   else
>     return CAT_CUTE_FACTOR;
> 
> 
> Your check would warn here, but it is possible, if not likely that the definition of those macros will eventually change. Again, it's fine not to cover this, but this //is// a false positive in a sense.

Yes, and it is potentially even build-configuration dependent. This is one of the big issues with macros that are impossible(?) to overcome. I would accept this case in the check. (Making it clear in the doc would not hurt).



================
Comment at: docs/clang-tidy/checks/list.rst:13
    abseil-str-cat-append
+   abseil-string-find-startswith
    android-cloexec-accept
----------------
donat.nagy wrote:
> JonasToth wrote:
> > spurious change
> It was added automatically by the python script, which sorts this list alphabetically. If I remove it now, it will be automatically re-added later.
yes. please remove it still, the python script is only run in `add_new_check.py`, i will commit the fix isolated to master.


================
Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
----------------
donat.nagy wrote:
> JonasToth wrote:
> > could you please add tests for ternary operators?
> Currently the check does not handle ternary operators, but I added some checks reflecting this.
Thank you. Could you please add a short note to the user-facing documentation that these are not handled.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757