[PATCH] D101060: [Analyzer][StdLibraryFunctionsChecker] Describe arg constraints

[Gabor Marton via Phabricator via cfe-commits]

martong marked 3 inline comments as done.
martong added a comment.

Thanks for the review Valeriy!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:338-339
         }
+        llvm_unreachable("The constraint must be either a concrete value or "
+                         "encoded in an argument.");
       }();
----------------
vsavchenko wrote:
> Just a thought here, maybe we should assert `SizeArgN` instead then?
Absolutely, good point.


================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp:32
+    __buf_size_arg_constraint_concrete(buf); // \
+    // expected-note{{The size of the 0th arg should be equal to or less than the value of 10}} \
+    // expected-warning{{}}
----------------
vsavchenko wrote:
> Oof, I do understand that we are devs and enumerate things starting from 0. But this is supposed to be human-readable and humans start counting from 1.
I've been thinking a lot about this and I see your point. On the other hand, we report warnings to other developers/programmers who are all used to start the indexing from 0, they may find it odd to start from 1. 

Alas, the string `0th` makes it obvious that we are talking about the first argument, however the string `1st` is ambiguous, even if we start the indexing from 0 or from 1. In this sense, starting from 0 makes less confusion.


================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:33
   // report-warning{{Function argument constraint is not satisfied}} \
+  // report-note{{}} \
   // bugpath-warning{{Function argument constraint is not satisfied}} \
----------------
vsavchenko wrote:
> What's up with these?
The new explanation of the constraints is added as an extra 'note' tag, which is displayed alongside the warning.
In these tests I don't want to test for the note, that would make these tests overly specified. For testing the notes we have a separate, newly added test file `std-c-library-functions-arg-constraints-notes.cpp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101060