[PATCH] D113863: [clang-tidy] Make `readability-container-data-pointer` more robust

[Fabian Wolff via Phabricator via cfe-commits]

fwolff marked 3 inline comments as done.
fwolff added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:76
+                    arraySubscriptExpr(hasLHS(ContainerExpr), hasRHS(Zero))))))
+          .bind(AddressOfName),
       this);
----------------
compnerd wrote:
> Nice, I do like this, it is quite a bit more succinct.  One thing that is a bit less clear to me is that the previous expression was a bit more restrictive in the parameter types to certain expressions, is there a reason that you don't expect that to matter much in practice?  If a malformed input is provided, we could now match certain things that we didn't previously, or am I not matching up the conditions carefully enough?
I have indeed changed things slightly to make everything more consistent. For instance, calling `&vp->operator[](0)` currently does not yield a warning ([[ https://godbolt.org/z/M1hGvGdWr | Godbolt link ]]), but `&v.operator[](0)` does, because the old check allowed a pointer to a container for a `cxxOperatorCallExpr()` (line 62 on the left) but not for `cxxMemberCallExpr()`. I think this was simply an oversight because the check was written in such a redundant way; my version covers both.

So yes, I have made some checks a bit less restrictive, but I think that's an improvement. Or which cases were you thinking of exactly?


================
Comment at: clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp:99
+      Lexer::getSourceText(CharSourceRange::getTokenRange(SourceRange),
+                           *Result.SourceManager, getLangOpts()));
+
----------------
compnerd wrote:
> The diff shows up odd here, is there a hard tab or is that just the rendering?
I think this is Phabricator's way of saying that the only thing that was changed in this line is the indentation. I haven't used any tabs.


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

https://reviews.llvm.org/D113863