Page MenuHomePhabricator

[clang-tidy] Add the abseil-time-comparison check
ClosedPublic

Authored by hwright on Mar 5 2019, 8:35 AM.

Details

Summary

This is an analog of the abseil-duration-comparison check, but for the absl::Time domain. It has a similar implementation and tests.

Diff Detail

Event Timeline

hwright created this revision.Mar 5 2019, 8:35 AM
Eugene.Zelenko added inline comments.
docs/clang-tidy/checks/abseil-time-comparison.rst
7

Please synchronize with Release Notes.

11

Please fix double spaces. Same below.

Eugene.Zelenko edited projects, added Restricted Project; removed Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 8:10 AM
ioeric added inline comments.Mar 6 2019, 8:55 AM
clang-tidy/abseil/TimeComparisonCheck.cpp
24

DurationComparisonCheck.cpp has a very similar matcher pattern.

Maybe pull out a common matcher for this? E.g. comparisonOperatorWithCallee(...)?

43

Move this comment closer the replacement logic below?

47

nit: double negation is a bit hard to read. maybe !(isNotInMacro(LSH) && isNotInMacro(RHS))? Ideally, the helper should be isInMacro instead of isNotInMacro.

hwright updated this revision to Diff 189862.Mar 8 2019, 8:12 AM
hwright marked 6 inline comments as done.
hwright added inline comments.
clang-tidy/abseil/TimeComparisonCheck.cpp
24

My one concern about doing so is that it would move the name bindings into a separate location away from the callback consuming those bindings.

Since this is only the second instance of this pattern, would it be reasonable to wait until there's a third to combine them?

ioeric accepted this revision.Mar 11 2019, 4:22 AM
ioeric added inline comments.
clang-tidy/abseil/TimeComparisonCheck.cpp
24

My one concern about doing so is that it would move the name bindings into a separate location away from the callback consuming those bindings.

I think the name binding can stay in the same file. comparisonOperatorWithCallee is a matcher for a FunctionDecl, so, on the call site, you could do comparisonOperatorWithCallee(functionDecl(TimeConversionFunction()).bind("function_decl")).bind("binop").

This revision is now accepted and ready to land.Mar 11 2019, 4:22 AM
hwright updated this revision to Diff 190075.Mar 11 2019, 6:13 AM
hwright marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.