[PATCH] D38764: [Analyzer] Assume const string-like globals are non-null

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.

Looks good to me. Please fix the additional nits mentioned inline and commit!  Also, make sure to do a pass to update the capitalization of variables throughout the file to match the LLVM coding standards. https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly



================
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:1
+//==-- RetainCountChecker.cpp - Checks for leaks and other issues -*- C++ -*--//
+//
----------------
The file name and description here needs to be updated for this checker.


================
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:10
+//
+//  Class definition for NonnullStringConstantsChecker.
+//  This checker adds an assumption that constant string-like globals are
----------------
I don't think the comment in the first line here is needed.


================
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:22
+//  Checker uses are defined in the test file:
+//   - test/Analysis/nonnull-string-constants.mm
+//
----------------
We generally don't do this kind of cross-reference in comments since they tend to get stale fast when things get moved around. There is no tool support to keep them in sync.


================
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:26
+
+//
+// This checker ensures that const string globals are assumed to be non-null.
----------------
Can this comment go?


================
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:53
+private:
+  /// Lazily initialize cache for required identifier informations.
+  void initIdentifierInfo(ASTContext &Ctx) const;
----------------
We usually put the oxygen comments on checkers on the implementation and not the interface since they aren't API and people reading the code generally look at the implementations. Can you move them to the implementations?


================
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:114
+  // Look through the typedefs.
+  while (const TypedefType *T = dyn_cast<TypedefType>(type)) {
+    type = T->getDecl()->getUnderlyingType();
----------------
To match the coding standards this should be `auto *` as well.


================
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:131
+
+  if (const ObjCObjectPointerType *T = dyn_cast<ObjCObjectPointerType>(type)) {
+    return T->getInterfaceDecl()->getIdentifier() == NSStringII;
----------------
Similar comment about `auto *`


================
Comment at: lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp:133
+    return T->getInterfaceDecl()->getIdentifier() == NSStringII;
+  } else if (const TypedefType *T = dyn_cast<TypedefType>(type)) {
+    return T->getDecl()->getIdentifier() == CFStringRefII;
----------------
Similar comment about `auto *`.


================
Comment at: test/Analysis/nonnull-string-constants.mm:5
+// Relies on the checker defined in
+// lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp.
+
----------------
We generally don't put file paths like this in comments since they tend to go stale. It is fine to to say that these are tests for the NonnullStringConstantsChecker though.


https://reviews.llvm.org/D38764