[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

[Gabor Marton via Phabricator via cfe-commits]

martong planned changes to this revision.
martong marked 4 inline comments as done.
martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:836-837
+          NewState, NewNode,
+          C.getNoteTag([Msg](PathSensitiveBugReport &BR,
+                             llvm::raw_ostream &OS) { OS << Msg; }));
     }
----------------
steakhal wrote:
> This way each and every applied constraint will be displayed even if the given argument does not constitute to the bug condition.
> I recommend you branching within the lambda, on the interestingness of the given argument constraint.
Okay, good point, thanks for the feedback! I am planning to change to this direction.


================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp:16
+int test_note(int x, int y) {
+    __single_val_1(x);  // expected-note{{Applied constraint: The 1st arg should be within the range [1, 1]}}
+    return y / (1 - x); // expected-warning{{Division by zero}} \
----------------
NoQ wrote:
> This has to be a user-friendly message.
> * "Constraints" is compiler jargon.
> * We cannot afford shortening "argument" to "arg".
> * Generally, the less machine-generated it looks the better (":" is definitely robotic).
Okay, thanks for your comment. I can make it to be more similar to the other notes we already have. What about this?
```
Assuming the 1st argument is within the range [1, 1]
```

> We cannot afford shortening "argument" to "arg".
I'd like to address this in another following patch if you don't mind.


================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:42
   int ret = isalnum(x);
+  // bugpath-note at -1{{Applied constraint: The 1st arg should be within the range [[-1, -1], [0, 255]]}}
   (void)ret;
----------------
NoQ wrote:
> This isn't part of this patch but what do you think about `{-1} U [0, 255]`? Or, you know, `[-1, 255]`.
Yeah, good idea, `{-1} U [0, 255]` would be indeed nicer and shorter. However, `[-1, 255]` is hard(er) so I am going to start with the former in a follow up patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101526