[PATCH] D144003: [clang][analyzer] Improve bug reports of StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Please run this on open source projects and upload the results.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:164-166
+    /// This function is called when the current constraint represents the
+    /// opposite of a constraint that was not satisfied in a given state, but
+    /// the opposite is satisfied. In this case the available information in the
----------------
I know what you mean, but this could use an example.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:168-170
+    /// description about the original constraint violation. This can be get by
+    /// try to narrow the current constraint while it remains satisfied in the
+    /// given program state. If useful information is found it is put into
----------------
This sentence makes so sense, unfortunately :( Could you rephrase, and, again, support it with an example?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:803
+    if (ValuesPrinted)
+      MsgOs << " that is out of the accepted range; It should be ";
+    else
----------------
Looking at the tests, this adds very little how about this:
" that is out of the accepted range; It should be " -> ", but should be "

Do you agree? I won't die on this hill, and am willing to accept this is good as-is if you really think that it is.

The other case is fine.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:807
+    VC->describe(Call, C.getState(), Summary, MsgOs);
+    // NegatedVC->describeBug1(Call, N->getState(), Summary, MsgOs);
     Msg[0] = toupper(Msg[0]);
----------------
Did you mean to leave this here?


================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp:18
   __not_null(nullptr); // \
-  // expected-warning{{The 1st argument to '__not_null' should not be NULL}}
+  // expected-warning{{The 1st argument to '__not_null' is NULL that is out of the accepted range; It should be not NULL [}}
 }
----------------
This english is broken, but more importantly, this is the one scenario where the original message was just good enough -- in fact, better. How about either
1. Restoring the original message (by somehow excluding `NotNullConstraint` in the new `describe()`)
2. Or saying something like "The 1st argument to '__not_null' is NULL, but should be non-NULL"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144003/new/

https://reviews.llvm.org/D144003