By using optionals, we no longer have to check the validity of types that we
get from a lookup. This way, the definition of the summaries have a declarative
form, there are no superflous conditions in the source code.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/test/Analysis/std-c-library-functions-POSIX-lookup.c | ||
---|---|---|
15 | Probably I should use FileCheck with --allow-empty and with CHECK-EMPTY |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
862 | Why has this class different layout than GetPointerTy and GetRestrictTy (beneath that here is no ASTContext)? It would be better if all these classes look the same way: First is the operator with QualType, then operator with Optional that calls the other version of the operator. And all of these can be "unnamed" classes? | |
901–902 | Is it better to use ACtx.VoidPtrTy? | |
1344 | It is better to get every type at the start before adding functions, or group the functions and get some types at the start of these groups but mark the groups at least with comments. |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
862 | I'd had the same thoughts before, and was thinking about that maybe a class template would suffice for all these, but then I realized the following: To make a type const, we don't need the ASTContext, that is the reason of the difference.
GetPointerTy and GetRestrictTy need the ASTContext in their constructor. And you cannot define a constructor to an unnamed class because you cannot write down the nonexistent name. | |
901–902 | I'd like to use the ACtx as less as possible: just to get the basic types. And from that on we can use our convenience API (GetConstTy, GetPointerTy, etc) to define all of the derived types... and we can do that in a declarative form (like ASTMatchters). | |
1344 | Well, with looked-up types I followed the usual convention to define a variable right before using it. This means that we lookup a type just before we try to add the function which first uses that type. However, builtin types are defined at the beginning, because they are used very often. |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
1017 | This return of empty vector and possibility of adding empty vector to range constraint is a new thing, probably it is better to check at create of range constraint (or other place) for empty range (in this case the summary could be made invalid)? But this occurs probably never because the matching type (of the max value) should be used at the same function and if the type is there the max value should be too. | |
1344 | I still like it better if all the type variables are created at one place (can be more easy to maintain if order changes and we have one block of types and one of functions) but this is no reason to block this change. |
- Add assert in getRanges
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
1017 | Alright, I added an assert to RangeConstraint::getRanges: const IntRangeVector &getRanges() const { + // When using max values for a type, the type normally should be part of + // the signature. Thus we must had looked up previously the type. If the + // type is not found then the range would be empty, but then the summary + // should be invalid too. + assert(Args.size() && "Empty range is meaningless"); return Args; } | |
1344 | I see your point, still I'd keep this way, because I this way the functions and the types they use are close to each other in the source. |
Just realized. There maybe cases when we'd like to give XMax to an arg constraint, but the type of the arg is Y (X may be a looked up type). One example for such is getchar, where the return type is Int, but we have a range constraint {0, UCharRangeMax}. Consequently, it seems to be better to remove the assert, because we will not be able to handle such cases with looked up types and their max values.
Why has this class different layout than GetPointerTy and GetRestrictTy (beneath that here is no ASTContext)? It would be better if all these classes look the same way: First is the operator with QualType, then operator with Optional that calls the other version of the operator. And all of these can be "unnamed" classes?