Page MenuHomePhabricator

[clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'
ClosedPublic

Authored by astrelni on Oct 29 2018, 1:18 PM.

Details

Summary

Introduce a new check to upgrade user code based on upcoming API breaking changes to absl::Duration.

The check finds calls to arithmetic operators and factory functions for absl::Duration that rely on an implicit user defined conversion to int64_t. These cases will no longer compile after proposed changes are released. Suggested fixes explicitly cast the argument int64_t.

Diff Detail

Event Timeline

astrelni created this revision.Oct 29 2018, 1:18 PM
astrelni retitled this revision from [clang-tidy]: Abseil: new check 'upgrade-duration-conversions' to [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'.Oct 29 2018, 1:25 PM
Eugene.Zelenko added inline comments.
clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
23

Please add check for C++. See other Abseil checks code as example.

137

Please don't use auto, since return type is not obvious.

docs/ReleaseNotes.rst
70

Please sort new checks list alphabetically.

astrelni updated this revision to Diff 171566.Oct 29 2018, 1:53 PM

Reply to comments: add check for language being c++, explicitly name type in declaration, alphabetize release notes.

astrelni marked 2 inline comments as done.Oct 29 2018, 1:55 PM

Hi astrelni,

my 2cents.
Please upload the patch will full context (i believe diff -U 99999, but check the man pages if that doesn't work :D)

clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
33

Please add types in your examples, to make clear what actually happens.

119

What happens on invalid ranges? Are they considered equal, or is it forbidden to pass them in?

124

Please add a test-case where the Node.getSourceRange() is a macro, if not already existing. I believe that is currently missing.

135

ast_matchers:: is not necessary, as there is a using ... at the top of the file.

159

You could ellide these braces, but I feel that this matching code be merged into one matcher with equalsBoundNode() (see ASTMatcher reference).

165

my personal preference would be removing the builtin-array and add two << FixitHint call to the diag.

clang-tidy/abseil/UpgradeDurationConversionsCheck.h
20

please mark code construct with ' in comments.

docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
44

please remove the last empty line

docs/clang-tidy/checks/list.rst
13

spurious changes in this file.

test/clang-tidy/abseil-upgrade-duration-conversions.cpp
143

what about non-type template parameters? Do they need consideration for the check as well?
If i am not mistaken floats are allowed in newwer standards as well.

astrelni updated this revision to Diff 171925.Oct 31 2018, 8:22 AM
astrelni marked 9 inline comments as done.

Thanks for the feedback so far.

Reply to review comments.

  • Improve comments.
  • Fix matcher to check for invalid source range.
  • Improve test coverage for templates and macros.
astrelni marked an inline comment as done.Oct 31 2018, 8:23 AM
astrelni added inline comments.
clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
119

Good point. I think this is a non-issue with the way the code is set up now, but better make it explicitly work :). We only care about valid source ranges. I've updated the name of the matcher and added a check here for the Node's source range and another in the matcher below to return false early.

124

Yes, thank you, that was missing. Added a few macro + template interactions.

159

Started with removing braces.

Sorry I had a look at equalsBoundNode(), but couldn't see exactly what you meant. Could you please elaborate about the merging?

test/clang-tidy/abseil-upgrade-duration-conversions.cpp
143

IIUC non-type template parameters should be no different for this check. This particular case is to make sure explicit specializations are treated differently from instantiations and get fix-it hints.

JonasToth added inline comments.Oct 31 2018, 9:18 AM
clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
159

i do not use that matcher on a daily basis, so take it with a grain of salt, please :)

prototype:

match(unless(allOf(isInTemplateInstantiation(equalsBoundNode("arg"))),
                                 expr(isInstantiationOfDependentExpr(equalsBoundNode("call")))))

You could do that matching in the registerMatchers even. The equalsBoundNode is suitable to connect different parts of a complexer matcher.
If you want, you can explore it. Slight simplification would be desirable here, but if it's not feasable the current form works too.

astrelni updated this revision to Diff 171973.Oct 31 2018, 11:15 AM

Combine template instantiation matching into the other matcher registration.

astrelni marked 3 inline comments as done.Oct 31 2018, 11:18 AM
astrelni added inline comments.
clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
159

Got it work out in registerMatchers and without needing equalsBoundNode, thanks for prodding me in that direction. I tried it initially when writing the check and can't remember why I didn't get it to work last time.

JonasToth added inline comments.Oct 31 2018, 11:49 AM
clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
168

Minor, but you can move diag(...) out the if condition like this:

auto Diag = diag(...);

and remove the duplication in the diag creation. The << works the same on the variable.

test/clang-tidy/abseil-upgrade-duration-conversions.cpp
54

Please add tests where the RHS is a complex arithmetic expression.

d = number1 + number2 + number3;

And the like, that trigger the warning and please ensure, the provided transformation is correct.

143

Please test in that case that the behaviour is as expected.

aaron.ballman added inline comments.Oct 31 2018, 12:51 PM
clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
152–153

How about: "implicit conversion to 'int64_t' is deprecated in this context; use an explicit cast instead"

docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
17

floating point -> floating-point

23

compile error -> a diagnostic

43

floating point -> floating-point

astrelni updated this revision to Diff 172000.Oct 31 2018, 1:17 PM
astrelni marked 2 inline comments as done.

Reply to review comments:

  • minor code reorder
  • improve test coverage
test/clang-tidy/abseil-upgrade-duration-conversions.cpp
143

Added some cases. I hope I interpreted correctly that "in that case" meant for template specializations / instantiations for non type template parameters.

astrelni updated this revision to Diff 172005.Oct 31 2018, 1:35 PM
astrelni marked 3 inline comments as done.

Reply to comments:

  • Change diagnostic message
  • Update documentation
docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
23

Tried rewording for clarity, let me know what you think. It doesn't seem like diagnostic is what's needed here as its talking about the upcoming API changes to Abseil and not the affects of this check.

astrelni added inline comments.Oct 31 2018, 3:42 PM
clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
152–153

Forgot to say thanks, this is much better :)

alexfh added inline comments.Oct 31 2018, 4:50 PM
clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
33–36

The hasAncestor and hasDescendant matchers frequently have non-trivial performance implications. Especially the latter, especially when called on a large number of nodes with a large number of transitive children (this obviously depends on the code being analyzed). I don't know how likely is this to cause problems here (the matcher seems to only be used when the check is about to issue a diagnostic), but it's always good to be aware of the possible issues.

Frequently there's a more efficient (and easier to understand) alternative. For example, instead of trying to evaluate certain conditions while traversing the code (which may require a large number of lookups into relatively distant parts of the AST), it's sometimes more efficient to split the analysis into two or more stages. During the AST traversal (i.e. when AST matchers run) the check could collect some raw information about all potentially problematic places in the code and then (for example, in onEndOfTranslationUnit) analyze the collected information together in a second stage. This works best if there's a way to arrange the data gathered on the first pass such that the second pass can efficiently look up necessary information.

136–141

Use hasAnyName instead.

astrelni updated this revision to Diff 172402.Nov 2 2018, 11:18 AM

Updated filtering of template instantiations to not use potentially costly hasDescendent matcher.

astrelni marked an inline comment as done.Nov 2 2018, 11:25 AM
astrelni added inline comments.
clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
33–36

Thanks, gave it a try, what do you think of this?

alexfh added inline comments.Nov 12 2018, 2:35 AM
clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
33–36

From a cursory look this should be less likely to be sloooow. In any case, makes sense to profile the check on a number of large files (at least against other checks, using clang-tidy's -enable-check-profile option).

44

hasAnyName, please. It's more efficient. A few more instances below.

astrelni updated this revision to Diff 174218.Nov 15 2018, 9:01 AM
astrelni marked an inline comment as done.

Fix to use hasAnyName everywhere

astrelni updated this revision to Diff 174226.Nov 15 2018, 9:30 AM

Fix incorrect uploaded diff.

alexfh accepted this revision.Nov 16 2018, 1:51 AM

LG with one more comment. Please address other reviewers' comments though.

clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
154

MatchedTemplateLocations can be a std::set<SourceLocation> or a std::unordered_set<unsigned> filled with SourceLocation::getRawEncoding(). Same for MatchedInstantiationLocations, I suppose.

This revision is now accepted and ready to land.Nov 16 2018, 1:51 AM
astrelni updated this revision to Diff 174931.Nov 21 2018, 7:45 AM
astrelni marked 5 inline comments as done.Nov 21 2018, 7:59 AM

Sorry for the long delay.

I've reworked the template instantiation stuff a little bit yet again. Going to still come back and comment with results of profiling but I think now this shouldn't be much slower than if the template instantiations were filtered out with the matchers.

clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
154

Ah thank you, I didn't look hard enough after not seeing a std::hash specialization for SourceLocation. In that case we don't need the onEndOfTranslationUnit or MatchedInstantiationLocations so those are now gone. I added a test case for when a template is used in source before the body since I wasn't 100% sure that the template would be traversed before the instantiation.

hokein added a subscriber: hwright.Dec 7 2018, 2:57 AM

+ @hwright who have implemented a bunch of absl-duration-* checks, you might be interested in this as well.

EricWF closed this revision.Dec 7 2018, 12:06 PM
EricWF added a subscriber: EricWF.

Committed as r348633.