dcoughlin added a comment.
Looks like a great start!
There are a bunch of minor nits inline.
The one big thing is that I think your handling of 'const char *' in
`typeIsConstString()` isn't quite right. 'const char *' means that the
pointed-to characters can't be modified but does allow modification of the
variable holding the pointer. I don't think we want to treat such variables as
holding non-null pointers, since anyone could assign them to NULL. On the other
hand, we do want to treat variables of type 'char * const' as holding non-null
pointers since the variable can't be reassigned and (presumably) it was
initialized to a non-null value. In this second case the characters could
potentially be modified, but that doesn't change whether the value of the
pointer itself.
================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:1
+
+//
----------------
We need to have the license preamble here.
================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:3
+//
+// This checker ensures that const string globals are assumed to be non-null.
+//
----------------
It would be good to include a motivation for why this is the right thing to do
and even an example of the declarations this will trigger on.
================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:17
+
+class NonnilStringConstantsChecker : public Checker<check::Location> {
+ mutable IdentifierInfo *NSStringII = nullptr;
----------------
Since this applies to more than just Objective-C, I think it would be better to
use 'null' instead of 'nil' in the name of the checker. Or even remove the
'Nonnil' prefix. What about 'GlobalStringConstantsChecker'?
================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:22
+public:
+ NonnilStringConstantsChecker(AnalyzerOptions &AO) {}
+
----------------
Does the constructor need to take an AnalyzerOptions?
================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:53
+ SVal V = UnknownVal();
+ if (location.isValid()) {
+ V = State->getSVal(location.castAs<Loc>());
----------------
Should we just early return if `location` is not valid?
================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:70
+
+bool NonnilStringConstantsChecker::isGlobalConstString(SVal val) const {
+ Optional<loc::MemRegionVal> regionVal = val.getAs<loc::MemRegionVal>();
----------------
Can you add doxygen-style comments to the implementations of these methods? I'd
like to break with tradition and have comments for all new code.
================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:71
+bool NonnilStringConstantsChecker::isGlobalConstString(SVal val) const {
+ Optional<loc::MemRegionVal> regionVal = val.getAs<loc::MemRegionVal>();
+ if (!regionVal)
----------------
Note that we use capital letters for variables and parameters in Clang/LLVM.
================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:74
+ return false;
+ const VarRegion *region = dyn_cast<VarRegion>(regionVal->getAsRegion());
+ if (!region)
----------------
The convention Clang uses with dyn_cast and friends to to use 'auto *' in the
variable declaration in order to not repeat the type name:
```
auto *Region = dyn_cast<VarRegion>(RegionVal->getAsRegion());
================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:93
+ // to be classified const.
+ hasOuterConst |= type.isConstQualified();
+ if (typeIsConstString(type, hasOuterConst))
----------------
Do you really want a bit-wise or here?
================
Comment at: lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp:113
+ II = T->getDecl()->getIdentifier();
+ }
+
----------------
This will leave `II` as uninitialized if your `if/else if` is not exhaustive.
Are you sure that it is?
================
Comment at: test/Analysis/nonnil-string-constants.mm:41
+// For char* we do not require a pointer itself to be immutable.
+extern const char *CharStarConst;
+void charStarCheckGlobal() {
----------------
What is the rationale for treating `const char *v` as non null?
In this scenario `v` can be reassigned, right?
================
Comment at: test/Analysis/nonnil-string-constants.mm:45
+}
+
+// But the pointed data should be.
----------------
I'd also like to see a test for treating `char * const v;` as non null.
================
Comment at: test/Analysis/nonnil-string-constants.mm:55
+extern str globalStr;
+void charStarCheckTypedef() {
+ clang_analyzer_eval(globalStr); // expected-warning{{TRUE}}
----------------
```
typedef const char *str;
extern str globalStr;
```
allows the `globalStr` variable to be written to.
Did you mean:
```
typedef char * const str;
extern str globalStr;
```
https://reviews.llvm.org/D38764
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
