mgrang updated this revision to Diff 163876.
mgrang added a comment.
Addressed comments.
https://reviews.llvm.org/D50488
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp
whisperity added a project: clang.
whisperity added a comment.
@Szelethus `clang-query` seems to sometimes not include matcher functions that
are perfectly available in the code... I recently had some issue with using
`isUserDefined()`, it was available in my code, but `clang-query` always
whisperity added a comment.
Minor comments in-line, looking good on first glance.
I'm not sure if we discussed, has this checker been tried on some in-the-wild
examples? To see if there are some real findings or crashes?
There is a good selection of projects here in this tool:
Szelethus added a comment.
In https://reviews.llvm.org/D50488#1204655, @mgrang wrote:
> In https://reviews.llvm.org/D50488#1203876, @Szelethus wrote:
>
> >
>
>
> This is my first time with ASTMatchers and I am not sure how to get the
> value_type from hasType (I don't see a matcher for
mgrang added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:72-76
+ auto IteratesPointerKeysM = hasType(cxxRecordDecl(has(
+fieldDecl(hasType(hasCanonicalType(
+
mgrang added a comment.
In https://reviews.llvm.org/D50488#1203876, @Szelethus wrote:
> I think testcases for non-class iterator objects would be valuable.
Thanks @Szelethus. Yes, as you correctly pointed out this would not match
non-class iterator objects.
This is my first time with
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:72-76
+ auto IteratesPointerKeysM = hasType(cxxRecordDecl(has(
+fieldDecl(hasType(hasCanonicalType(
+
Szelethus added a comment.
I think testcases for non-class iterator objects would be valuable.
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:72-76
+ auto IteratesPointerKeysM = hasType(cxxRecordDecl(has(
+
mgrang updated this revision to Diff 161165.
mgrang edited the summary of this revision.
mgrang added a comment.
Added checks for more algorithms: stable_sort, is_sorted, partial_sort,
partition, stable_partition, nth_element.
https://reviews.llvm.org/D50488
Files:
mgrang added a comment.
This was my first time using AST matchers so it took me a while to figure out
how exactly to get this right. clang-query helped a lot. Backspace seems to be
a problem with clang-query though.
https://reviews.llvm.org/D50488
mgrang updated this revision to Diff 161152.
mgrang added a comment.
Changed patch to use AST Matchers.
https://reviews.llvm.org/D50488
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
NoQ added a comment.
I guess one of the things the analyzer could find with path-sensitive analysis
is direct comparison of non-aliasing pointers. Not only this is
non-deterministic, but there's a related problem that comparison for equality
would always yield false and is therefore useless.
mgrang added a comment.
Thanks for all your review comments. I will try to address them soon.
Meanwhile, I have started an email thread on the general problem of writing
checkers/sanitizers to detect non-deterministic behaviors. See
NoQ added a comment.
> I'm only a beginner, but here are some things that caught my eye. I really
> like the idea! :)
Thanks, i appreciate help with reviews greatly.
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:93
+ const RecordDecl *RD =
+
george.karpenkov requested changes to this revision.
george.karpenkov added a comment.
This revision now requires changes to proceed.
All the comments above + if anything, should use ASTMatchers.
Repository:
rC Clang
https://reviews.llvm.org/D50488
whisperity added a comment.
The basics of the heuristics look okay as comparing pointers from
non-continuous block of memory is undefined, it would be worthy to check if no
compiler warning (perhaps by specifying `-W -Wall -Wextra -Weverything` and
various others of these enable-all flags!) is
Szelethus added a comment.
I'm only a beginner, but here are some things that caught my eye. I really like
the idea! :)
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:28
+
+// PointerSortingVisitor class.
+class PointerSortingVisitor : public StmtVisitor {
whisperity added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:88
+
+ if (!II->getName().equals("sort"))
+return;
Brrr... `equals`. StringRef has a `==` and `!=` operator which accepts string
literals on the other side,
Szelethus added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/PointerSortingChecker.cpp:91-100
+ const QualType IterTy = CE->getArg(0)->getType();
+ const RecordDecl *RD =
+IterTy->getUnqualifiedDesugaredType()->getAsCXXRecordDecl();
+
+ if (RD->field_empty())
NoQ added a comment.
Hmm, this might make a good `optin.*` checker. Also i see that this is a pure
AST-based checker; did you consider putting this into `clang-tidy`? Like, you
shouldn't if you're not using `clang-tidy` yourself (it's a bit messy) but this
is an option. Also we're actively
mgrang created this revision.
mgrang added reviewers: NoQ, george.karpenkov, whisperity.
Herald added subscribers: mikhail.ramalho, a.sidorin, rnkovacs, szepet,
xazax.hun, mgorny.
Repository:
rC Clang
https://reviews.llvm.org/D50488
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
21 matches
Mail list logo
