Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Off-topic: I really think that we should have some sort of DSL for that kind of stuff.
| clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
|---|---|---|
| 2054 | It feels like the readability of the code here can be drastically improved by introducing functions getPointerType, getRestrictType, and similar accepting Optional arguments. | |
In a sense the API provided in the Checker itself is an (embedded) DSL. Or do you think about being able to provide the summaries by describing them in a JSON or a YAML file?
| clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
|---|---|---|
| 2054 | Yeah, we already have getRestrictTy, but never though to have getPointerTy, which indeed simplifies the code, thanks! | |
Even some macro-like construct can improve readability. The current way of adding functions is source of copy-paste errors because more things are repeated, these should be written only once. And the ifs for the existence of types can be eliminated (automated) somehow. So the final way of specifying the functions is to only list (get) the types first, then add the functions with name, arguments and constraints in a way that nothing needs to be repeated and computable parts are done automatically.
| clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
|---|---|---|
| 2054 | I think the improvement would be if these get functions accept Optional arguments and if the argument is empty return an empty value. This can make the ifs unnecessary. | |
Yes, this totally makes sense, thank you guys for this suggestion!
Now I've started working on to reach this form, what do you think?
// Taking an optional as arg.
Optional<QualType> Pthread_cond_tPtrTy = getPointerTy(Pthread_cond_tTy);
addToFunctionSummaryMap(
{"pthread_cond_signal", "pthread_cond_broadcast"},
// Create a signature with Optionals, addToFunctionSummaryMap will skip
// the summary if the optional is not set.
Signature(ArgTypes{Pthread_cond_tPtrTy}, RetType{IntTy}),
Summary(NoEvalCall)
.ArgConstraint(NotNull(ArgNo(0))));Side note, one day hopefully we could have the same API to fill a CallDescriptionMap.
Now I've update the patch to remove all the ifs. And the optional types are now implicitly handled, so no such mistakes can be done that I had done previously.
However, the patch is getting to affect the core of this Checker, so, perhaps I should split this into two patches? One NFC for the core changes and another that involves only the addition of the pthread functions?
It is better now, but the refactoring change should be in a separate revision (it affects the other already added functions too).
| clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
|---|---|---|
| 379 | Not very bad but I do not like to call another constructor and assignment here, this can be many redundant operations. It is better to fill the internal arrays here and have a separate assert function that checks for validness (of the internal arrays) instead of the private constructor. | |
| 401 | Check for isInvalid here (with assert)? | |
| 955 | Another idea: Have a class that handles all the variants (simple, pointer, const pointer, const restrict pointer, restrict). It can have get functions that compute the type if not done yet, or get every variant at first time even if it is later not used (or has no sense). | |
| 1072–1073 | These ifs can be removed too (in another patch). | |
Thanks @balazske for your comments, you always make an assiduous review!
| clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
|---|---|---|
| 379 | Ok, makes sense, thanks! | |
| 401 | We cannot do that, this is not a member function of the Signature, this operates on a FuncitonDecl. But it makes sense to add the assert to the mathces member function. | |
| 955 | I'd rather keep the current form, because I think it makes it easier to compose types from different base types (and it is similar to the ASTMatchers in this sense). | |
| 1072–1073 | Yep, I am going to create another base patch. | |
| clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
|---|---|---|
| 955 | If I got it correctly, the problem with my approach above is that it has separate functions for every special type variant. The current is acceptable but looks not fully uniform to me (the type has a withConst and for getting a pointer or restrict there is a function call). We could have something like a "TypeBuilder" that has methods withConst(), withRestrict(), withPointerTo() (these can return a TypeBuilder too) and finally a (omittable) getType() that returns Optional<QualType>. const QualType ConstVoidPtrRestrictTy = TypeBuilder{ACtx, VoidTy}.withConst().withPointerTo().withRestrict().getType(); | |
| clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
|---|---|---|
| 955 | Yeah, .withConst() is making it nonuniform currently, you are right. Optional<QualType> ConstPthread_attr_tPtrTy = getPointerTy(getConstTy(Pthread_attr_tTy)); WDYT? | |
@balazske seems to be very involved, he might have some closing words -- from my end, LGTM!
Not very bad but I do not like to call another constructor and assignment here, this can be many redundant operations. It is better to fill the internal arrays here and have a separate assert function that checks for validness (of the internal arrays) instead of the private constructor.