[PATCH] D41077: [analyser] different.CallArgsOrder checker implementation

[Aleksei Sidorin via Phabricator via cfe-commits]

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