[analyser] different.CallArgsOrder checker implementation

[alexey.knyshev]

Summary

CallArgsOrderChecker which looks for accidental swap or skip of arguments in function, methods, constructors and operators calls

[anna]

Perhaps added me incorrectly as reviewer?

[szepet]

Hi Alexey!

Thank you for working on this!

Some general comments on the patch:

1. Please upload the changes with the context included (git flag -U99999) which could help the review process.
2. Use the LLVM Coding Guidelines on the tests as well (Start variable names with a capital letter, etc )
3. Ping here, not necessary on the cfe-dev mailing list ;)
4. FYI: There is a similar check under review which uses only the AST provided information and implemented as a tidy-checker: https://reviews.llvm.org/D20689 (As I see your checker does not uses symbolic execution provided features. So, it would probably worth thinking about if the analyzer is the project where the checker should be implemented. However, @dcoughlin and @alexfh have more insight on the answer to this question. What do you think? )
5. In overall, please add comments to the function which describes its purpose. Mainly on heuristic functions, it can help to understand it more easily. Also, could you provide some results of the current heuristics, what are the false positive rates on real projects like LLVM, FFmpeg, etc? I am quite interested in that.

Some comments added inline but they are basically the above mentioned things.

lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp

57

It looks like that Levenshtein edit distance could come handy in cases like this. It is already implemented as a method edit_distance on StringRef.

164

Please add a comment to this function which describes its input, output, purpose.

test/Analysis/call-args-order.c

4

Please use capital starting letter on variables. On the other test file as well.

[alexfh]

In D41077#958819, @szepet wrote:

4. FYI: There is a similar check under review which uses only the AST provided information and implemented as a tidy-checker: https://reviews.llvm.org/D20689 (As I see your checker does not uses symbolic execution provided features. So, it would probably worth thinking about if the analyzer is the project where the checker should be implemented. However, @dcoughlin and @alexfh have more insight on the answer to this question. What do you think? )

The main problem with the check proposed in https://reviews.llvm.org/D20689 (which uses, IIUC, very similar algorithm to this one) is that the false positive rate on real projects is too high. The comment I made on the other review thread applies here as well:

There's definitely potential in using parameter name and argument spelling to detect possibly swapped arguments, and there's a recent research on this topic, where authors claim to have reached the true positive rate of 85% with decent recall. See https://research.google.com/pubs/pub46317.html. If you're interested in working on this check, I would suggest at least looking at the techniques described in that paper.

As for whether this analysis should be implemented in the static analyzer or as a clang-tidy check: I think, clang-tidy would be more suitable, since

1. this analysis doesn't need the path analysis machinery provided by the static analyzer
2. this analysis may benefit from suggesting automated fixes
3. the false positive rate of the state-of-the-art algorithm implementing similar analysis have a false positive rate of at least 15-30%, and a large fraction of false positives could potentially be treated as "code smells" which can be fixed by giving function parameters or local variables proper names. I don't know whether this kind of analysis meets the bar the static analyzer sets for its checkers. Anna or Devin can tell more here.

5. In overall, please add comments to the function which describes its purpose. Mainly on heuristic functions, it can help to understand it more easily. Also, could you provide some results of the current heuristics, what are the false positive rates on real projects like LLVM, FFmpeg, etc? I am quite interested in that.

You can take the list of projects from https://reviews.llvm.org/D20689#878803, run the analysis on them, and see whether the results are any better.

[a.sidorin]

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.

lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp

31

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.

33

You don't need to reserve the amount of items same or less then SmallVector default size - they are already stack-allocated.

112

else if?

119

trim* functions should be moved out of ParamsGroup because they are not related.

139

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().

144

What you are trying to do here (find common prefix of two strings) can be done easier using std::mismatch().

157

llvm::transform(StrVec, StrVec.begin(), ...)

164

With names like removeCommon[Pre/Suf]fix, the intention of these functions will be much cleaner. Also, please do not pass vectors by value.

185

We can use std::mismatch with std::reverse_iterator (std::rbegin(), std::rend()).

208

We can use try_emplace() to construct the new item in-place.

215

This is already check in the caller. I think, this can be replaced with an assertion.

257

Could you please explain what kind of ExprName are you trying to get?

259

As I remember, IgnoreParenCasts() can not return nullptr. So, we can turn this into assertion.

262

OrigE is not nullptr so we can use dyn_cast.

278

We have ASTContext so we can use getPrintingPolicy() and getLangOpts() instead of default values.

279

We can use Lexer::getSourceText() here.

311

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.

319

The function is pretty long. I think we should split it.

323

Should we check for NumArgs <= FirstActualArgOffset + 1, like in condition below?

334

This will look nicer with C++11-style loop.

363

Should we return or just continue?

415

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).

test/Analysis/call-args-order.c

13

Please mark tests for true negatives as // no warning.

include/clang/StaticAnalyzer/Checkers/Checkers.td

   HelpText<"Warn about assigning non-{0,1} values to Boolean variables">,
   DescFile<"BoolAssignmentChecker.cpp">;
 
+def CallArgsOrderChecker : Checker<"CallArgsOrder">,
+  HelpText<"Warn about accidental args swap in various call expressions">,
+  DescFile<"CallArgsOrderChecker.cpp">;
+
 def CastSizeChecker : Checker<"CastSize">,
   HelpText<"Check when casting a malloc'ed type T, whether the size is a multiple of the size of T">,
   DescFile<"CastSizeChecker.cpp">;

lib/StaticAnalyzer/Checkers/CMakeLists.txt

   CStringChecker.cpp
   CStringSyntaxChecker.cpp
   CallAndMessageChecker.cpp
+  CallArgsOrderChecker.cpp
   CastSizeChecker.cpp
   CastToStructChecker.cpp
   CheckObjCDealloc.cpp

lib/StaticAnalyzer/Checkers/CallArgsOrderChecker.cpp

+//= CallArgsOrderChecker.cpp - Check for arguments order in calls -*- C++ -*-=//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This defines CallArgsOrderChecker, which looks for accidental swap or skip
+// of arguments in function, methods, constructors and operators calls.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ExprCXX.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+namespace {
+constexpr unsigned TypicalMaxParamsCount = 4;
+typedef SmallVector<StringRef, TypicalMaxParamsCount> StringRefVec;
+
+class ParamsGroup {
+public:
+  ParamsGroup() : StartIndex(0) { ParamsVec.reserve(4); }
+
+  static StringRefVec trimSame(StringRefVec StrVec);
+  static StringRefVec ltrimSame(StringRefVec StrVec);
+  static StringRefVec rtrimSame(StringRefVec StrVec);
+
+  void fillSimplifiedParamNames() {
+    StringRefVec ParamNames;
+    ParamNames.reserve(ParamsVec.size());
+    for (const Param &P : ParamsVec)
+      ParamNames.push_back(P.Decl->getName());
+
+    if (ParamNames.size() > 1)
+      ParamNames = trimSame(ParamNames);
+
+    for (StringRefVec::size_type i = 0, N = ParamsVec.size(); i < N; i++)
+      ParamsVec[i].SimplifiedName = ParamNames[i];
+  }
+
+  class Param {
+  public:
+    const ParmVarDecl *Decl;
+    StringRef SimplifiedName;
+
+    bool isNamedLike(StringRef Name) const {
+      if (SimplifiedName.size() < Name.size()) {
+        if (Name.startswith_lower(SimplifiedName))
+          return true;
+      } else {
+        if (SimplifiedName.startswith_lower(Name))
+          return true;
+      }
+
+      // We don't like false-positive matches on single-char
+      if (SimplifiedName.size() < 2 || Name.size() < 2)
+        return false;
+
+      if (SimplifiedName.size() < Name.size())
+        return Name.endswith_lower(SimplifiedName);
+      return SimplifiedName.endswith_lower(Name);
+    }
+  };
+
+  unsigned StartIndex;
+  SmallVector<Param, TypicalMaxParamsCount> ParamsVec;
+};
+
+typedef llvm::SmallVector<ParamsGroup, TypicalMaxParamsCount> ParamsGroupVec;
+
+class CallArgsOrderChecker : public Checker<check::ASTCodeBody> {
+  mutable llvm::DenseMap<const FunctionDecl *, ParamsGroupVec> PGLCache;
+
+public:
+  const ParamsGroupVec &getOrBuildParamsGroups(const FunctionDecl *FD) const;
+  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
+                        BugReporter &BR) const;
+};
+
+class Callback : public MatchFinder::MatchCallback {
+  const CallArgsOrderChecker *C;
+  BugReporter &BR;
+  SourceManager &SM;
+  AnalysisDeclContext *AC;
+
+  void checkCallExpr(const CallExpr *CE);
+  void checkCXXConstructExpr(const CXXConstructExpr *CE);
+
+  template <typename CallExprT>
+  void checkCall(const FunctionDecl *FD, const CallExprT *CE,
+                 unsigned FirstActualArgOffset) const;
+
+public:
+  Callback(const CallArgsOrderChecker *C, BugReporter &BR, SourceManager &SM,
+           AnalysisDeclContext *AC)
+      : C(C), BR(BR), SM(SM), AC(AC) {}
+
+  virtual void run(const MatchFinder::MatchResult &Result) override {
+    if (const CallExpr *CE = Result.Nodes.getNodeAs<CallExpr>("callExpr"))
+      checkCallExpr(CE);
+    if (const CXXConstructExpr *CE =
+            Result.Nodes.getNodeAs<CXXConstructExpr>("cxxConstructExpr"))
+      checkCXXConstructExpr(CE);
+  }
+};
+} // end of anonymous namespace
+
+StringRefVec ParamsGroup::trimSame(StringRefVec StrVec) {
+  return rtrimSame(ltrimSame(StrVec));
+}
+
+StringRefVec ParamsGroup::ltrimSame(StringRefVec StrVec) {
+  if (StrVec.empty())
+    return StrVec;
+
+  auto It = StrVec.begin();
+  const auto End = StrVec.end();
+
+  StringRef SamePrefix = *It;
+  It++;
+
+  for (; It != End; It++) {
+    const StringRef &Str = *It;
+
+    const size_t StrSize = Str.size();
+    size_t SamePrefixSize = SamePrefix.size();
+
+    if (SamePrefixSize > StrSize) {
+      SamePrefix = SamePrefix.drop_back(SamePrefixSize - StrSize);
+      SamePrefixSize = SamePrefix.size();
+    }
+
+    for (size_t i = 0; i < SamePrefixSize; i++) {
+      if (Str[i] != SamePrefix[i]) {
+        SamePrefix = SamePrefix.drop_back(SamePrefixSize - i);
+        break;
+      }
+    }
+
+    // No common prefix found
+    if (SamePrefix.empty())
+      return StrVec;
+  }
+
+  const size_t SamePrefixSize = SamePrefix.size();
+  std::transform(StrVec.begin(), StrVec.end(), StrVec.begin(),
+                 [SamePrefixSize](const StringRef &Str) {
+                   return Str.drop_front(SamePrefixSize);
+                 });
+  return StrVec;
+}
+
+StringRefVec ParamsGroup::rtrimSame(StringRefVec StrVec) {
+  if (StrVec.empty())
+    return StrVec;
+
+  auto It = StrVec.begin();
+  const auto End = StrVec.end();
+
+  StringRef SameSuffix = *It;
+  It++;
+
+  for (; It != End; It++) {
+    const StringRef &Str = *It;
+
+    const size_t StrSize = Str.size();
+    size_t SameSuffixSize = SameSuffix.size();
+
+    if (SameSuffixSize > StrSize) {
+      SameSuffix = SameSuffix.drop_front(SameSuffixSize - StrSize);
+      SameSuffixSize = SameSuffix.size();
+    }
+
+    for (size_t i = StrSize, j = SameSuffixSize; i > 0 && j > 0; i--, j--) {
+      if (Str[i - 1] != SameSuffix[j - 1]) {
+        SameSuffix = SameSuffix.drop_front(j);
+        break;
+      }
+    }
+
+    // No common suffix found
+    if (SameSuffix.empty())
+      return StrVec;
+  }
+
+  const size_t SameSuffixSize = SameSuffix.size();
+  std::transform(StrVec.begin(), StrVec.end(), StrVec.begin(),
+                 [SameSuffixSize](const StringRef &Str) {
+                   return Str.drop_back(SameSuffixSize);
+                 });
+  return StrVec;
+}
+
+// Split function params in groups with same qualified types
+const ParamsGroupVec &
+CallArgsOrderChecker::getOrBuildParamsGroups(const FunctionDecl *FD) const {
+  const auto It = PGLCache.find(FD);
+  if (It != PGLCache.end())
+    return It->second;
+
+  ParamsGroupVec PGL;
+
+  const unsigned NumParams = FD->getNumParams();
+  if (NumParams > 0) {
+    const ParmVarDecl *prevParam = FD->getParamDecl(0);
+    ParamsGroup PG;
+
+    ParamsGroup::Param p;
+    p.Decl = prevParam;
+
+    PG.ParamsVec.push_back(std::move(p));
+
+    PGL.push_back(std::move(PG));
+
+    for (unsigned ParamIndex = 1; ParamIndex < NumParams; ParamIndex++) {
+      const ParmVarDecl *currParam = FD->getParamDecl(ParamIndex);
+
+      QualType prevType = prevParam->getOriginalType().getCanonicalType();
+      QualType currType = currParam->getOriginalType().getCanonicalType();
+
+      ParamsGroup::Param p;
+      p.Decl = currParam;
+
+      if (currType == prevType) {
+        PGL.back().ParamsVec.push_back(std::move(p));
+      } else {
+        ParamsGroup PG;
+        PG.StartIndex = ParamIndex;
+
+        PG.ParamsVec.push_back(std::move(p));
+
+        PGL.push_back(PG);
+      }
+
+      prevParam = currParam;
+    }
+
+    for (ParamsGroup &PG : PGL)
+      PG.fillSimplifiedParamNames();
+  }
+
+  auto InsertRes = PGLCache.insert(std::make_pair(FD, std::move(PGL)));
+  return InsertRes.first->second;
+}
+
+static StringRef getExprName(const Expr *E) {
+  const Expr *OrigE = E->IgnoreParenCasts();
+  if (!OrigE)
+    return StringRef();
+
+  if (const DeclRefExpr *DRE = dyn_cast_or_null<DeclRefExpr>(OrigE))
+    return DRE->getFoundDecl()->getName();
+  else if (const UnaryOperator *UO = dyn_cast_or_null<UnaryOperator>(OrigE))
+    return getExprName(UO->getSubExpr());
+  else if (const CallExpr *CE = dyn_cast_or_null<CallExpr>(OrigE))
+    return getExprName(CE->getCallee());
+  else if (const MemberExpr *ME = dyn_cast_or_null<MemberExpr>(OrigE))
+    return ME->getMemberDecl()->getName();
+
+  return StringRef();
+}
+
+static StringRef getExprString(const Expr *E, SourceManager &SM) {
+  SourceLocation Start(E->getLocStart());
+  SourceLocation End(E->getLocEnd());
+
+  End = Lexer::getLocForEndOfToken(End, 0, SM, LangOptions());
+  const char *Str = SM.getCharacterData(Start);
+  return StringRef(Str, SM.getCharacterData(End) - Str);
+}
+
+template <typename CallExprT>
+static unsigned getDefaultArgsCount(const CallExprT *CE) {
+  const unsigned NumArgs = CE->getNumArgs();
+  for (unsigned ArgIndex = 0; ArgIndex < NumArgs; ArgIndex++)
+    if (isa<CXXDefaultArgExpr>(CE->getArg(ArgIndex)))
+      return NumArgs - ArgIndex;
+  return 0;
+}
+
+void Callback::checkCallExpr(const CallExpr *CE) {
+  unsigned FirstActualArgOffset = 0;
+  if (const CXXOperatorCallExpr *OCE =
+          dyn_cast_or_null<CXXOperatorCallExpr>(CE)) {
+    if (OCE->getOperator() != OO_Call)
+      return;
+
+    // Skipping first implicit object argument
+    FirstActualArgOffset = 1;
+  }
+
+  const FunctionDecl *FD = CE->getDirectCallee();
+  if (!FD)
+    return;
+
+  checkCall(FD, CE, FirstActualArgOffset);
+}
+
+void Callback::checkCXXConstructExpr(const CXXConstructExpr *CE) {
+  const FunctionDecl *FD = CE->getConstructor();
+  if (!FD)
+    return;
+
+  checkCall(FD, CE, /*FirstActualArgOffset=*/0);
+}
+
+template <typename CallExprT>
+void Callback::checkCall(const FunctionDecl *FD, const CallExprT *CE,
+                         unsigned FirstActualArgOffset) const {
+  // We are not interested in calls without arguments
+  const unsigned NumArgs = CE->getNumArgs();
+  if (NumArgs <= FirstActualArgOffset)
+    return;
+
+  const unsigned NumParams = FD->getNumParams();
+  // Args order permutation isn't possible if FD has < 2 params
+  if (NumParams <= 1)
+    return;
+
+  const unsigned DefNumArgs = getDefaultArgsCount(CE);
+
+  const ParamsGroupVec &PGV = C->getOrBuildParamsGroups(FD);
+  for (ParamsGroupVec::size_type PGVIndex = 0, PGVSize = PGV.size();
+       PGVIndex < PGVSize; PGVIndex++) {
+    const ParamsGroup &PG = PGV[PGVIndex];
+
+    const unsigned PVSize = PG.ParamsVec.size();
+    if (PVSize < 2)
+      continue;
+
+    StringRefVec SimplifiedArgNames;
+    SimplifiedArgNames.reserve(PVSize);
+    for (unsigned PVIndex = 0; PVIndex < PVSize; PVIndex++) {
+      const unsigned ArgIndex = PG.StartIndex + PVIndex + FirstActualArgOffset;
+      SimplifiedArgNames.push_back(getExprName(CE->getArg(ArgIndex)));
+    }
+
+    SimplifiedArgNames = ParamsGroup::trimSame(SimplifiedArgNames);
+
+    const unsigned InvalidMismatchIndex = ~unsigned(0);
+    unsigned FirstMismatchIndex = InvalidMismatchIndex;
+
+    for (unsigned PVIndex = 0; PVIndex < PVSize - 1; PVIndex++) {
+      const ParamsGroup::Param &CurrParam = PG.ParamsVec[PVIndex];
+      const ParamsGroup::Param &NextParam = PG.ParamsVec[PVIndex + 1];
+
+      StringRef CurrArgExprName = SimplifiedArgNames[PVIndex];
+      StringRef NextArgExprName = SimplifiedArgNames[PVIndex + 1];
+
+      // Skip invalid simplified names
+      if (CurrParam.SimplifiedName.empty() || CurrArgExprName.empty())
+        return;
+
+      // Looks like arg matches param
+      if (CurrParam.isNamedLike(CurrArgExprName))
+        continue;
+
+      const unsigned ArgIndex = PG.StartIndex + PVIndex + FirstActualArgOffset;
+      const Expr *CurrArg = CE->getArg(ArgIndex);
+      const Expr *NextArg = CE->getArg(ArgIndex + 1);
+
+      if (FirstMismatchIndex == InvalidMismatchIndex)
+        FirstMismatchIndex = PG.StartIndex + PVIndex;
+
+      if (NextParam.isNamedLike(CurrArgExprName)) {
+        PathDiagnosticLocation Loc =
+            PathDiagnosticLocation::createBegin(CurrArg, SM, AC);
+
+        SmallString<128> Message;
+        llvm::raw_svector_ostream OS(Message);
+
+        // Looks like arg for CurrParam is missing
+        // and args sequence is shifted towards the left
+        if (DefNumArgs > 0 && PG.StartIndex + PVIndex + 1 == NumParams - 1) {
+          OS << "possibly missing arg for param '"
+             << FD->getParamDecl(FirstMismatchIndex)->getName() << "', arg '"
+             << getExprString(CE->getArg(FirstMismatchIndex), SM)
+             << "' passed instead";
+          BR.EmitBasicReport(AC->getDecl(), C, "Possibly missing argument",
+                             categories::LogicError, Message, Loc,
+                             CurrArg->getSourceRange());
+          break;
+          // Looks like args are swapped
+        } else if (CurrParam.isNamedLike(NextArgExprName)) {
+          OS << "possibly wrong order of args '" << getExprString(CurrArg, SM)
+             << "' and '" << getExprString(NextArg, SM)
+             << "' passed as params '" << CurrParam.Decl->getName() << "' and '"
+             << NextParam.Decl->getName() << "'";
+          SourceRange Sr[2];
+          Sr[0] = CurrArg->getSourceRange();
+          Sr[1] = NextArg->getSourceRange();
+          BR.EmitBasicReport(AC->getDecl(), C,
+                             "Possibly wrong order of arguments",
+                             categories::LogicError, Message, Loc, Sr);
+        }
+      }
+    }
+  }
+}
+
+void CallArgsOrderChecker::checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
+                                            BugReporter &BR) const {
+  auto CallExpr = stmt(
+      eachOf(forEachDescendant(callExpr().bind("callExpr")),
+             forEachDescendant(cxxConstructExpr().bind("cxxConstructExpr"))));
+
+  MatchFinder F;
+  Callback CB(this, BR, Mgr.getSourceManager(), Mgr.getAnalysisDeclContext(D));
+
+  F.addMatcher(CallExpr, &CB);
+  F.match(*D->getBody(), Mgr.getASTContext());
+}
+
+namespace clang {
+namespace ento {
+void registerCallArgsOrderChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<CallArgsOrderChecker>();
+}
+} // namespace ento
+} // namespace clang

test/Analysis/call-args-order.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.CallArgsOrder -verify -std=c99 %s
+
+struct Viewport {
+  int width;
+  int height;
+
+  float aspectRatio;
+};
+
+void setDimentions(struct Viewport *vp, int width, int height) {
+  vp->width = width;
+  vp->height = height;
+  vp->aspectRatio = width / height;
+}
+
+float getWidth(struct Viewport *vp) { return vp->width; }
+float getHeight(struct Viewport *vp) { return vp->height; }
+
+float getFloat() {
+  return 128.5;
+}
+
+int main() {
+  struct Viewport vp;
+
+  int newWidth = 800;
+  int newHeight = 600;
+  setDimentions(&vp, newHeight, newWidth);    // expected-warning {{possibly wrong order of args 'newHeight' and 'newWidth' passed as params 'width' and 'height'}}
+  setDimentions(&vp, newWidth, newHeight);    // no-warning
+  setDimentions(&vp, newWidth, newWidth);     // no-warning
+  setDimentions(&vp, newHeight, newHeight);   // no-warning
+  setDimentions(&vp, getFloat(), getFloat()); // no-warning
+
+  int w = 640;
+  int h = 480;
+  setDimentions(&vp, w, h); // no-warning
+  setDimentions(&vp, h, w); // expected-warning {{possibly wrong order of args 'h' and 'w' passed as params 'width' and 'height'}}
+  setDimentions(&vp, w, w); // no-warning
+  setDimentions(&vp, h, h); // no-warning
+
+  int w_big = 1920;
+  int h_big = 1080;
+
+  float w_big_f = 320.0;
+  float h_big_f = 240.0;
+
+  setDimentions(&vp, w_big, h_big); // no-warning
+  setDimentions(&vp, h_big, w_big); // expected-warning {{possibly wrong order of args 'h_big' and 'w_big' passed as params 'width' and 'height'}}
+  setDimentions(&vp, w_big, w_big); // no-warning
+  setDimentions(&vp, h_big, h_big); // no-warning
+
+  setDimentions(&vp, (int)h_big_f, -w_big_f);            // expected-warning {{possibly wrong order of args '(int)h_big_f' and '-w_big_f' passed as params 'width' and 'height'}}
+  setDimentions(&vp, (int)h_big_f, h_big_f);             // no-warning
+  setDimentions(&vp, -getHeight(&vp), getWidth(&vp));    // expected-warning {{possibly wrong order of args '-getHeight(&vp)' and 'getWidth(&vp)' passed as params 'width' and 'height'}}
+  setDimentions(&vp, -getHeight(&vp), getHeight(&vp));   // no-warning
+  setDimentions(&vp, getHeight(&vp) + 4, getWidth(&vp)); // no-warning
+}

test/Analysis/call-args-order.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.CallArgsOrder -verify -x c++ %s
+
+typedef unsigned char byte;
+
+class Color {
+  byte r, g, b, a;
+
+public:
+  Color(byte R, byte G, byte B, byte A = 0)
+      : r(R), g(G), b(B), a(A) {}
+
+  void set(byte R, byte G, byte B, byte A = 0) {
+    r = R;
+    g = G;
+    b = B;
+    a = A;
+  }
+
+  void operator()(byte R, byte G, byte B, byte A = 0);
+  void operator+=(byte Delta);
+
+  byte getR() const { return r; }
+  byte getG() const { return g; }
+  byte getB() const { return b; }
+  byte getA() const { return a; }
+
+  static Color copy1(const Color &Color);
+  static Color copy2(const Color &Color);
+  static Color copy3(const Color &Color);
+  static Color copy4(const Color &Color);
+  static Color copy5(const Color &Color);
+  static Color copy6(const Color &Color);
+};
+
+void Color::operator()(byte R, byte G, byte B, byte A) {
+  set(R, B, G, A); // expected-warning {{possibly wrong order of args 'B' and 'G' passed as params 'G' and 'B'}}
+}
+
+void Color::operator+=(byte Delta) {
+  r += Delta;
+  g += Delta;
+  b += Delta;
+  a += Delta;
+}
+
+Color newRed1(byte r, byte b, byte a) {
+  return Color(r, b, a); // expected-warning {{possibly missing arg for param 'G'}}
+}
+
+Color newRed2(byte r, byte g, byte a) {
+  return Color(r, g, a); // expected-warning {{possibly missing arg for param 'B'}}
+}
+
+Color Color::copy1(const Color &o) {
+  return Color(o.r, o.b, o.g, o.a); // expected-warning {{possibly wrong order of args 'o.b' and 'o.g' passed as params 'G' and 'B'}}
+}
+
+Color Color::copy2(const Color &o) {
+  return Color(o.r, o.g, o.a, o.b); // expected-warning {{possibly wrong order of args 'o.a' and 'o.b' passed as params 'B' and 'A'}}
+}
+
+Color Color::copy3(const Color &o) {
+  return Color(o.r, o.g, o.g, o.g); // no-warning
+}
+
+Color Color::copy4(const Color &o) {
+  return Color(o.r, o.r, o.r, o.r); // no-warning
+}
+
+Color Color::copy5(const Color &o) {
+  return Color(o.getR(), o.getB(), o.getG(), o.getA()); // expected-warning {{possibly wrong order of args 'o.getB()' and 'o.getG()' passed as params 'G' and 'B'}}
+}
+
+Color Color::copy6(const Color &o) {
+  // totaly messed up (not sure if it's wrong or not)
+  return Color(o.a, o.r, o.g, o.b); // no-warning
+}
+
+Color newMagicColor() {
+  const byte r = 200;
+  const byte g = 150;
+  const byte b = 100;
+  const byte a = 0;
+
+  return Color(r, b, g, a); // expected-warning {{possibly wrong order of args 'b' and 'g' passed as params 'G' and 'B'}}
+}
+
+void modify(Color &c) {
+  int red = 1;
+  int green = 2;
+  int blue = 3;
+  c.set(red, green, blue);     // no-warning
+  c.set(red, blue, green);     // expected-warning {{possibly wrong order of args 'blue' and 'green' passed as params 'G' and 'B'}}
+  c.set(red, green, 0);        // no-warning
+  c.set(red, green, c.getA()); // no-warning
+
+  c(red, green, blue); // no-warning
+  c(green, red, blue); // expected-warning {{possibly wrong order of args 'green' and 'red' passed as params 'R' and 'G'}}
+
+  c += 1; // no-warning
+
+  c(red, blue, green, 0); // expected-warning {{possibly wrong order of args 'blue' and 'green' passed as params 'G' and 'B'}}
+}