This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] StdLibraryFunctionsChecker refactor: remove macros
ClosedPublic

Authored by martong on Feb 3 2020, 8:17 AM.

Diff Detail

Event Timeline

martong created this revision.Feb 3 2020, 8:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2020, 8:17 AM

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.

NoQ added a comment.Feb 3 2020, 9:45 AM

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).

533–547

I suspect that this comment would need a lot more updates.

549

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).

martong marked 3 inline comments as done.Feb 7 2020, 2:48 AM
martong added inline comments.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
533–547

Could you please elaborate? Do you mean to add comments e.g. to ArgumentCondition and the rest below? Or to rewrite the above comment?

549

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.
Removed the T suffix too from the lambdas that can generate summaries (e.g. Getc).

martong updated this revision to Diff 243117.Feb 7 2020, 2:48 AM
martong marked an inline comment as done.
  • Rename several types
Szelethus accepted this revision.Feb 7 2020, 4:16 AM

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.

This revision is now accepted and ready to land.Feb 7 2020, 4:16 AM
martong updated this revision to Diff 243167.Feb 7 2020, 7:26 AM
martong marked 6 inline comments as done.
  • 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.

Szelethus added inline comments.Feb 7 2020, 7:41 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
42–49

Totally agreed! Thank you for the cleanup!

martong marked 2 inline comments as done.Feb 10 2020, 6:05 AM

Ping @NoQ

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
42–49

No, thanks for the review! :)

NoQ accepted this revision.Feb 10 2020, 6:47 AM

LGTM, thanks again!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
533–547

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.).

598

Just curious, can RetType also use curly braces?

martong updated this revision to Diff 243552.Feb 10 2020, 7:12 AM
martong marked 7 inline comments as done.
  • Ditch comment about (macro) format
  • Use {} with RetType

Thanks for the review guys!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
533–547

Ok, I ditched it.

598

Yes. I've changed it to use the curly braces with RetType too, now the format is more consistent with ArgTypes.

This revision was automatically updated to reflect the committed changes.