a.sidorin added a comment.
Hi Alexey,
This commit strongly needs testing on some real code. I cannot predict the TP
rate of this checker now.
Regarding implementation, you can find some remarks inline.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:31
+
+class ParamsGroup {
+public:
----------------
We have a class without any private parts. This means that we should declare it
as 'struct' or do some redesign - the way it is constructed (with expressions
like `PGL.back().ParamsVec.push_back(std::move(p));` clearly indicates some
design issues. I suggest at least adding a wrapper above ParamsGroupVec with
methods:
`ParamGroup &addParam(const ParmVarDecl *ParamVD)`; // adds a variable into
existing group or creates new one
`void reclaimLastGroupIfSingle(); // deletes last added group if it has only a
single element`
and moving some logic here.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:33
+public:
+ ParamsGroup() : StartIndex(0) { ParamsVec.reserve(4); }
+
----------------
You don't need to reserve the amount of items same or less then SmallVector
default size - they are already stack-allocated.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:112
+ checkCallExpr(CE);
+ if (const CXXConstructExpr *CE =
+ Result.Nodes.getNodeAs<CXXConstructExpr>("cxxConstructExpr"))
----------------
else if?
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:119
+
+StringRefVec ParamsGroup::trimSame(StringRefVec StrVec) {
+ return rtrimSame(ltrimSame(StrVec));
----------------
trim* functions should be moved out of ParamsGroup because they are not
related.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:139
+
+ if (SamePrefixSize > StrSize) {
+ SamePrefix = SamePrefix.drop_back(SamePrefixSize - StrSize);
----------------
We can the code:
```
SamePrefix = SamePrefix.take_front(StrSize);
SamePrefixSize = SamePrefix.size();
```
instead. The behaviour of `StringRef::take_front(size_t N)` is well-defined if
N >= size().
Same below: drop_front() can be replaced with take_back().
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:144
+
+ for (size_t i = 0; i < SamePrefixSize; i++) {
+ if (Str[i] != SamePrefix[i]) {
----------------
What you are trying to do here (find common prefix of two strings) can be done
easier using std::mismatch().
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:157
+ const size_t SamePrefixSize = SamePrefix.size();
+ std::transform(StrVec.begin(), StrVec.end(), StrVec.begin(),
+ [SamePrefixSize](const StringRef &Str) {
----------------
llvm::transform(StrVec, StrVec.begin(), ...)
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:164
+
+StringRefVec ParamsGroup::rtrimSame(StringRefVec StrVec) {
+ if (StrVec.empty())
----------------
szepet wrote:
> Please add a comment to this function which describes its input, output,
> purpose.
With names like removeCommon[Pre/Suf]fix, the intention of these functions will
be much cleaner. Also, please do not pass vectors by value.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:185
+
+ for (size_t i = StrSize, j = SameSuffixSize; i > 0 && j > 0; i--, j--) {
+ if (Str[i - 1] != SameSuffix[j - 1]) {
----------------
We can use std::mismatch with std::reverse_iterator (std::rbegin(),
std::rend()).
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:208
+CallArgsOrderChecker::getOrBuildParamsGroups(const FunctionDecl *FD) const {
+ const auto It = PGLCache.find(FD);
+ if (It != PGLCache.end())
----------------
We can use `try_emplace()` to construct the new item in-place.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:215
+ const unsigned NumParams = FD->getNumParams();
+ if (NumParams > 0) {
+ const ParmVarDecl *prevParam = FD->getParamDecl(0);
----------------
This is already check in the caller. I think, this can be replaced with an
assertion.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:257
+
+static StringRef getExprName(const Expr *E) {
+ const Expr *OrigE = E->IgnoreParenCasts();
----------------
Could you please explain what kind of ExprName are you trying to get?
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:259
+ const Expr *OrigE = E->IgnoreParenCasts();
+ if (!OrigE)
+ return StringRef();
----------------
As I remember, IgnoreParenCasts() can not return nullptr. So, we can turn this
into assertion.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:262
+
+ if (const DeclRefExpr *DRE = dyn_cast_or_null<DeclRefExpr>(OrigE))
+ return DRE->getFoundDecl()->getName();
----------------
OrigE is not nullptr so we can use dyn_cast.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:278
+
+ End = Lexer::getLocForEndOfToken(End, 0, SM, LangOptions());
+ const char *Str = SM.getCharacterData(Start);
----------------
We have ASTContext so we can use getPrintingPolicy() and getLangOpts() instead
of default values.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:279
+ End = Lexer::getLocForEndOfToken(End, 0, SM, LangOptions());
+ const char *Str = SM.getCharacterData(Start);
+ return StringRef(Str, SM.getCharacterData(End) - Str);
----------------
We can use Lexer::getSourceText() here.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:311
+void Callback::checkCXXConstructExpr(const CXXConstructExpr *CE) {
+ const FunctionDecl *FD = CE->getConstructor();
+ if (!FD)
----------------
Personally, I think that writing:
```
if (const FunctionDecl *FD = CE->getConstructor())
checkCall(FD, CE, /*FirstActualArgOffset=*/0);
```
is shorter and clearer. But it is so minor that I don't insist on this change.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:319
+template <typename CallExprT>
+void Callback::checkCall(const FunctionDecl *FD, const CallExprT *CE,
+ unsigned FirstActualArgOffset) const {
----------------
The function is pretty long. I think we should split it.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:323
+ const unsigned NumArgs = CE->getNumArgs();
+ if (NumArgs <= FirstActualArgOffset)
+ return;
----------------
Should we check for `NumArgs <= FirstActualArgOffset + 1`, like in condition
below?
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:334
+ const ParamsGroupVec &PGV = C->getOrBuildParamsGroups(FD);
+ for (ParamsGroupVec::size_type PGVIndex = 0, PGVSize = PGV.size();
+ PGVIndex < PGVSize; PGVIndex++) {
----------------
This will look nicer with C++11-style loop.
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:363
+ if (CurrParam.SimplifiedName.empty() || CurrArgExprName.empty())
+ return;
+
----------------
Should we return or just continue?
================
Comment at: lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp:415
+ auto CallExpr = stmt(
+ eachOf(forEachDescendant(callExpr().bind("callExpr")),
+ forEachDescendant(cxxConstructExpr().bind("cxxConstructExpr"))));
----------------
Instead of `eachOf(forEachDescendant(), forEachDescendant())` it is better to
use `forEachDescendant(anyOf())`. This will avoid double iteration on
descendant tree (the memoizing size for descendant tree is limited).
================
Comment at: test/Analysis/call-args-order.c:13
+ vp->height = height;
+ vp->aspectRatio = width / height;
+}
----------------
Please mark tests for true negatives as `// no warning`.
Repository:
rC Clang
https://reviews.llvm.org/D41077
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
