[PATCH] D77493: [clang-tidy] Add do-not-refer-atomic-twice check

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/DoNotReferAtomicTwiceCheck.cpp:21
+  Finder->addMatcher(
+      declRefExpr(hasType(hasUnqualifiedDesugaredType(atomicType())),
+                  to(varDecl().bind("atomic")),
----------------
I have run the tests and I found that the last bad test case is not detected. I have played with clang-query to investigate and I have found that the main reason is the other `declRefExpr` that matches is not on the RHS but also on LHS, just one level deeper in the AST.

You could patch it further, but IMO that would surely become way too complex to understand and maintain in the long run. What I propose is to not try to solve everything with ASTMatchers. This problem is inherently symmetric, and there is no way match based on the complexity of an expression, which could be used a way to deduplicate results.

I propose the following:

use a matcher like this:
```
expr(
  hasDescendant(
    declRefExpr(
      to(
        varDecl(
          hasType(
            hasUnqualifiedDesugaredType(
              atomicType()
            )
          )
        ).bind("varDecl")
      )
    ).bind("one")
  ),
  hasDescendant(
    declRefExpr(
      to(
        varDecl(
          equalsBoundNode("varDecl")
        )
      ),
      unless(equalsBoundNode("one"))
    ).bind("other")
  )
);
```
This will match conflicting DeclRefs with every pair-combinations. Then you could use bound nodes `one` and `other` and a set data structure to deduplicate the reports as needed. Note you should also watch out for expressions where 3 references exist to the same VarDecl. That case would emit 6 diagnostics, 4 references 12, etc. (Also 4 references could result in the first 2 then the second two being processed, and you would still have duplication. Handling this is probably not really worth it, as I would think this case is unlikely in real-world code.)

I would consider this approach more customizable because you decide how to deduplicate the results. It is also more understandable, as deduplicating with Matchers is not really idiomatic in this symmetric case IMHO. I would not go as far as to say anything about performance, but I would be surprised to find a good, straightforward filtering solution using Matchers.



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D77493