dcoughlin added a comment.
In https://reviews.llvm.org/D20811#544981, @NoQ wrote:
> Hmm, what about
>
> CONSTRAIN
> ARGUMENT_VALUE(0, WithinRange)
> RANGE('0', '9')
> RANGE('A', 'Z')
> RANGE('a', 'z')
> END_ARGUMENT_VALUE
> RETURN_VALUE(OutOfRange)
>
NoQ added a comment.
In https://reviews.llvm.org/D20811#544927, @dcoughlin wrote:
> That said, now that I look at it with 'POSTCONDITION' alone I don't think it
> is clear that the provided value describes the return value. What do you
> think about renaming it 'RETURN_VALUE'? Or adding back
dcoughlin added a comment.
I think this is much clearer! That said, now that I look at it with
'POSTCONDITION' alone I don't think it is clear that the provided value
describes the return value. What do you think about renaming it 'RETURN_VALUE'?
Or adding back the RET_VAL I asked you about
NoQ added a comment.
In https://reviews.llvm.org/D20811#544250, @dcoughlin wrote:
> I think a good rule of thumb for readability is: suppose you are a maintainer
> and need to add a summary for a new function. Can you copy the the summary
> for an existing function and figure out what each
dcoughlin added a comment.
Thanks for adding the macros. I've provided some feedback inline.
I think a good rule of thumb for readability is: suppose you are a maintainer
and need to add a summary for a new function. Can you copy the the summary for
an existing function and figure out what
NoQ updated this revision to Diff 71510.
NoQ marked an inline comment as done.
NoQ added a comment.
Herald added subscribers: mgorny, beanz.
Added a huge amount of macros in order to improve readability of function specs.
Other inline comments should have been addressed before.
NoQ added a comment.
> Is it really a problem if the checker comments are part of the Doxygen
> documentation?
Of course not :) I've been mostly thinking about the benefits of the anonymous
namespace itself (cleaner global scope, no name collisions, but even these
benefits are extremely
Alexander_Droste added a comment.
> It has been originally written as a large set of files. If you feel strongly
> about it, we could merge it into a single file. That makes sense to me.
> @Alexander_Droste, what do you think?
Hi,
I would still strongly prefer to keep them in separate files
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:206
@@ +205,3 @@
+: Call.getArgExpr(ArgNo)->getType().getCanonicalType();
+ }
+ static QualType getArgType(const CallExpr *CE, ArgNoTy ArgNo) {
zaks.anna added a subscriber: Alexander_Droste.
zaks.anna added a comment.
> Even though there are some doxygen-style comments in the checkers, i’ve never
> seen doxygen actually generate any docs for checker classes.
> Are they useful for IDE quick-hints only?
I think it's useful to have
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:509
@@ +508,3 @@
+ //}
+ // }
+ //}
dcoughlin wrote:
> I disagree about compactness being valuable here. I think it is more
> important to intrinsically
dcoughlin added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:509
@@ +508,3 @@
+ //}
+ // }
+ //}
I disagree about compactness being valuable here. I think it is more important
to intrinsically document the spec.
NoQ added a comment.
Answering myself: Hmm, so the only reason why MPI checker class appears in
doxygen
(http://clang.llvm.org/doxygen/classclang_1_1ento_1_1mpi_1_1MPIChecker.html) is
because this class is not in anonymous namespace (as far as i understand, they
needed to be multi-file for
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:192
@@ +191,3 @@
+ };
+
+ // The map of all functions supported by the checker. It is initialized
Even though there are some doxygen-style comments in the
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:191
@@ +190,3 @@
+// impossible to verify this precisely, but at least
+// this check avoids potential crashes.
+bool matchesCall(const CallExpr *CE) const;
NoQ updated this revision to Diff 65369.
NoQ marked 4 inline comments as done.
https://reviews.llvm.org/D20811
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:191
@@ +190,3 @@
+// impossible to verify this precisely, but at least
+// this check avoids potential crashes.
+bool matchesCall(const CallExpr *CE) const;
NoQ updated this revision to Diff 65248.
NoQ marked 11 inline comments as done.
NoQ added a comment.
Renamed the checker as **xazax.hun** suggested, added a lot more comments, done
with inline comments :)
https://reviews.llvm.org/D20811
Files:
zaks.anna added a comment.
Thanks for the patch! Here are the comments, most of which are nits.
Could you add the high level description of what we are doing somewhere or
maybe just describe the meaning of FunctionSpec since it encodes how functions
are modeled.
Also, we should explain why we
NoQ added a comment.
Yeah, good point, the "Std" part should definitely appear in the name, not sure
about the "C" thing, as we could probably expand this checker to model some
simple C++ functions as well (and then we'd make a separate checker section to
move from unix. to cplusplus. or
xazax.hun added a comment.
It is great to model more widely used functions! However I think the
LibraryFunctionsChecker name might be a bit to broad, wouldn't something like
StdCLibraryFunctions be more informative?
http://reviews.llvm.org/D20811
21 matches
Mail list logo
