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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
