Szelethus added a comment.
The high level idea and the implementation of the checker seems great. In
general, things that you want to address in later patches should be stated in
the code with a `TODO`. I wrote a couple nits that I don't want to delete, but
maybe it'd be better to address them after the dependency patch is agreed upon.
================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:296
+def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">,
+ HelpText<"Check constraints of arguments of C standard library functions">,
+ Dependencies<[StdCLibraryFunctionsChecker]>,
----------------
How about we add an example as well?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:161
+
+ ValueRange negate() const {
+ ValueRange tmp(*this);
----------------
Maybe `complement` would be a better name? That sounds a lot more like a set
operation. Also, this function highlights well that inheritance might not be
the best solution here.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:191
+ /// * a list of branches - a list of list of ranges -
+ /// i.e. a list of lists of lists of segments,
+ /// * a list of argument constraints, that must be true on every branch.
----------------
I think that is a rather poor example to help understand what `list of list of
ranges` means :) -- Could you try to find something better?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:10
// This checker improves modeling of a few simple library functions.
// It does not generate warnings.
//
----------------
I suspect this comment is no longer relevant.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:399-407
+ auto Report = [&](ExplodedNode *N) {
+ if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
+ return;
+ // FIXME Add detailed diagnostic.
+ StringRef Msg = "Function argument constraint is not satisfied";
+ auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
+ bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
----------------
While I find your usage of lambdas fascinating, this one seems a bit
unnecessary :)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:402
+ return;
+ // FIXME Add detailed diagnostic.
+ StringRef Msg = "Function argument constraint is not satisfied";
----------------
That is a `TODO`, rather :^)
================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:1-7
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -triple x86_64-unknown-linux-gnu \
+// RUN: -verify
----------------
Hmm, why do we have 2 different test files that essentially do the same?
Shouldn't we only have a single one with `analyzer-output=text`?
================
Comment at: clang/test/Analysis/std-c-library-functions.c:1-31
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config eagerly-assume=false \
+// RUN: -triple i686-unknown-linux \
+// RUN: -verify
----------------
What a beautiful sight. Thanks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73898/new/
https://reviews.llvm.org/D73898
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
