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).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
1 | upsz |
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? |
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. |
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. |
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. |
upsz