Page MenuHomePhabricator

[Analyzer][StdLibraryFunctionsChecker] Describe arg constraints
ClosedPublic

Authored by martong on Thu, Apr 22, 6:18 AM.

Details

Summary

In this patch, I provide a detailed explanation for each argument
constraint. This explanation is added in an extra 'note' tag, which is
displayed alongside the warning.
Since these new notes describe clearly the constraint, there is no need
to provide the number of the argument (e.g. 'Arg3') within the warning.
However, I decided to keep the name of the constraint in the warning (but
this could be a subject of discussion) in order to be able to identify
the different kind of constraint violations easily in a bug database
(e.g. CodeChecker).

Diff Detail

Event Timeline

martong created this revision.Thu, Apr 22, 6:18 AM
martong requested review of this revision.Thu, Apr 22, 6:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Apr 22, 6:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
martong added inline comments.Thu, Apr 22, 6:19 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
1

upsz

martong updated this revision to Diff 339603.Thu, Apr 22, 6:27 AM
martong marked an inline comment as not done.
  • Put back first line
  • Remove wrong comment

Great job! Thanks!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
327–328

Just a thought here, maybe we should assert SizeArgN instead then?

clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp
33

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.

clang/test/Analysis/std-c-library-functions-arg-constraints.c
33

What's up with these?

martong marked 3 inline comments as done.Fri, Apr 23, 1:37 AM

Thanks for the review Valeriy!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
327–328

Absolutely, good point.

clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp
33

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.

clang/test/Analysis/std-c-library-functions-arg-constraints.c
33

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.

martong updated this revision to Diff 339934.Fri, Apr 23, 1:37 AM
martong marked 3 inline comments as done.
  • Assert on SizeArgN
vsavchenko added inline comments.Fri, Apr 23, 1:59 AM
clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp
33

I know that we are talking to developers, but no developers say that this is a 0th argument. And IMO the vast majority of developers would think of the argument at index 0 when they read '1st' because most of people are not compiler engineers and don't think of the list of arguments as an array.
But that is all opinions after all. What is most important is that clang already reports a ton of warnings pointing to a specific argument/parameter by its ordinal number. Simply grep DiagnosticsSemaKinds.td for ordinal and see the examples in tests. As you can guess, they all use ordinals starting from 1st.

martong marked an inline comment as done.Fri, Apr 23, 6:19 AM
martong added inline comments.
clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp
33

Fair enough. It would be inconsistent to start from 0 if the Sema warnings already start from 1. I've changed it.

martong updated this revision to Diff 339999.Fri, Apr 23, 6:19 AM
martong marked an inline comment as done.
  • Start arg index display from 1.

Awesome, thanks!

LGTM now!

vsavchenko accepted this revision.Fri, Apr 23, 6:30 AM
This revision is now accepted and ready to land.Fri, Apr 23, 6:30 AM