[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Hi!

Overall the check looks good to me, I have only one problem. We usually try to emit fixes if some parts of the replaced text is a macro, e.g.:

  #define MYMACRO(X) count(X)
  
  if (myCont.MYMACRO(X))
   ...

In the above code snippet, we want to avoid modifying the source inside the definition of `MYMACRO`, because that could render the code incorrect if `MYMACRO` is used in another context, e.g. with a custom container as well. I think most tidy checks are explicitly guarding the replacements to avoid these situations.

Once this is fixed and a test is added, the rest looks good to me.



================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:112
+  bool Negated = NegativeComparison != nullptr;
+  const auto *Comparison = Negated ? NegativeComparison : PositiveComparison;
+
----------------
Should we have an assert to express that only one of `PositiveComparison` and `NegativeComparison` expected to be not-null?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646