Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Unit tests: fail. 62099 tests passed, 5 failed and 784 were skipped.
failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
clang-tidy: pass.
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
This looks fantastic, i didn't think of this originally. All hail type safety!
Nits below.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
176 | Let's keep calling them "cases" because iirc this was a matter of discussion (https://reviews.llvm.org/D20811?id=71510#inline-211441). | |
530 | There seems to be a lot of inconsistency in the use of T, Ty, and lack-of-suffix. I'd prefer to have one of them reserved for QualTypes (eg., IntTy). | |
530–544 | I suspect that this comment would need a lot more updates. |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
530 | Ok, I've renamed every type with the Ty suffix to not have any suffix. Ty is reserved for variables now whose type is QualType, e.g. IntTy. | |
530–544 | Could you please elaborate? Do you mean to add comments e.g. to ArgumentCondition and the rest below? Or to rewrite the above comment? |
This is amazing. LGTM, granted that @NoQ is happy with the patch as well.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
32–33 | If we put pure in quotes, we might as well go the extra mile and quickly explain what that means. | |
42–49 | I would prefer to just have a checker option that could print out the currently modeled function rather than these lines of a recipe for outdated comments. | |
214–216 | But why? This isn't important for this patch, so feel free to leave as-is, but still. |
- Describe what pure means
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
32–33 | Ok, I removed the quotes and added the meaning of pure in parenthesis. | |
42–49 | Yes I agree, especially because I am planning to add a plethora of new functions in the future. I think that would be the appropriate time to implement the checker option. | |
214–216 | I suppose this is because we cannot have a reference to the ASTContext in the constructor of the Checker. We can get that only via any of the checker callbacks. |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
42–49 | Totally agreed! Thank you for the cleanup! |
LGTM, thanks again!
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
530–544 | Actually let's ditch it entirely. It was worth it when it was all macros, so that it was apparent how macros expanded, but now it's pretty self-explanatory all the way. Otherwise i was thinking about making this a pattern that the user can copy-paste and fill in. Like, maybe, include all the constructors explicitly (Summary, ArgTypes, etc.). | |
581 | Just curious, can RetType also use curly braces? |
If we put pure in quotes, we might as well go the extra mile and quickly explain what that means.