This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add the abseil-duration-factory-scale check
ClosedPublic

Authored by hwright on Nov 7 2018, 5:26 PM.

Details

Summary

This check removes unneeded scaling of arguments when calling Abseil Time factory functions.

Diff Detail

Event Timeline

hwright created this revision.Nov 7 2018, 5:26 PM
Eugene.Zelenko retitled this revision from Add the abseil-duration-factory-scale clang-tidy check to [clang-tidy] Add the abseil-duration-factory-scale check.Nov 7 2018, 5:38 PM
Eugene.Zelenko added a reviewer: aaron.ballman.
Eugene.Zelenko set the repository for this revision to rCTE Clang Tools Extra.
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added inline comments.
clang-tidy/abseil/DurationFactoryScaleCheck.cpp
36

This will cause memory leaks, so may be unique_ptr should be used to hold pointer? May be LLVM ADT has better container?

59

else after return. Same in other places.

164

Should be llvm_unreachable().

288

Please remove trailing return and comment above. See readability-redundant-control-flow.

docs/ReleaseNotes.rst
88

Second statement belongs to documentation.

docs/clang-tidy/checks/abseil-duration-factory-scale.rst
7

Please fix double space.

hwright updated this revision to Diff 173166.Nov 8 2018, 7:21 AM
hwright marked 4 inline comments as done.

Address reviewer comments

clang-tidy/abseil/DurationFactoryScaleCheck.cpp
36

This is a tradeoff between leaking a small amount of known memory for the duration of the program, and constructing this map every time this function is invoked. Since I expect the latter to occur frequently, that's a tradeoff I think is acceptable. (Ideally, this would be a compile-time constant, but sadly we don't yet have a constexpr dictionary type.)

Of course, if there is a more typical way of doing that here, I'm happy to use it.

docs/ReleaseNotes.rst
88

It already is in the user-facing documentation, are saying it should be removed from here?

Eugene.Zelenko added inline comments.Nov 8 2018, 10:12 AM
docs/ReleaseNotes.rst
88

Release Notes are for brief descriptions. Documentation is for details.

aaron.ballman added inline comments.Nov 8 2018, 10:15 AM
clang-tidy/abseil/DurationFactoryScaleCheck.cpp
36

Why did you not use a static value type?

hokein added inline comments.Nov 9 2018, 3:48 AM
clang-tidy/abseil/DurationFactoryScaleCheck.cpp
23

nit: wrap it in anonymous namespace?

36

I think main reason is lazy initialization.

75

we are comparing with floats, I think we should use something AlmostEquals(Multiplier, 60.0).

203

isn't Call->getExprLoc().isMacroID() enough?

267

is this for debugging?

aaron.ballman added inline comments.Nov 9 2018, 10:49 AM
clang-tidy/abseil/DurationFactoryScaleCheck.cpp
36

Static locals are lazily initialized, so I'm still not certain why this shouldn't use static const std::unorderd_map<std::string, DurationScale> ScaleMap{...}; instead.

47

Elide braces.

58

Elide braces.

61

Please add a message to the assertion. e.g., assert(foo && "bar");

180–182

::absl instead -- otherwise this can trigger in unintended places.

hwright updated this revision to Diff 173543.Nov 10 2018, 6:03 PM
hwright marked 16 inline comments as done.

Addressed reviewer feedback.

clang-tidy/abseil/DurationFactoryScaleCheck.cpp
75

I can't find AlmostEquals elsewhere; could you point me to it so I can investigate it's use?

But I am also curious why you think we should use this. We are checking for exact values. 60.0 is representable exactly as a double, and even values which aren't (e.g., 1e-3) will have the same representation in this function as they do in the code being transformed, so equality is still appropriate.

203

Thanks! I've also added tests to verify this.

aaron.ballman added inline comments.Nov 11 2018, 2:45 PM
clang-tidy/abseil/DurationFactoryScaleCheck.cpp
40

GetScaleForFactory -> getScaleForFactory, per naming conventions. Same for the other functions.

49

Drop top-level const (this returns an iterator, not a pointer/reference).

56

it's -> its

58–59

I really don't like this interface where you pass two arguments, only one of which is ever valid. That is pretty confusing. Given that the result of this function is only ever passed to GetNewMultScale(), and that function only does integral checks, I'd prefer logic more like:

  • If the literal is integral, get its value and call GetNewMultScale().
  • If the literal is float, convert it to an integral and call GetNewMultScale() only if the conversion is exact (this can be done via APFloat::convertToInteger()).
  • GetNewMultScale() can now accept an integer value and removes the questions about inexact equality tests from the function.

With that logic, I don't see a need for GetValue() at all, but if a helper function is useful, I'd probably guess this is a better signature: int64_t getIntegralValue(const Expr *Literal, bool &ResultIsExact);

60

Elide these braces.

64

I believe the approximate results here can lead to bugs where the floating-point literal is subnormal -- it may return 0.0 for literals that are not zero.

73

Hours with a multiplier of 1.0/60.0 would scale to minutes, wouldn't it?

82–85

What about scaling with a multiplier of 3600 to go from seconds to hours, and other plausible conversions?

113

Similar comments here as above. In fact, I feel like these functions should be combined into getNewScale() and have normalized the scaling value in the caller so that it covers both operations rather than manually spelling out the conversions in either direction. e.g., convert everything into multiplier form in the caller by treating divisors as a multiplication by 1/divisor.

151

This should return a StringRef.

195

and -> to

199

I suspect you want to ignore paren expressions as well, so IgnoreParenImpCasts().

214

Do not use auto here as the type is not explicitly spelled out in the initialization.

clang-tidy/abseil/DurationFactoryScaleCheck.h
20

funciton -> function

hwright updated this revision to Diff 173714.Nov 12 2018, 10:50 AM
hwright marked 11 inline comments as done.

Addressed small concerns.

Larger issues pending feedback.

clang-tidy/abseil/DurationFactoryScaleCheck.cpp
58–59

Given that the result of this function is only ever passed to GetNewMultScale(), and that function only does integral checks, I'd prefer logic more like:

That's actually not true: GetNewMultScale() does checks against values like 1e-3 which aren't integers. Does this change your suggestion?

64

Do you have an example which I could put in a test?

82–85

That's a good point, and part of a broader design discussion: should we support all multipliers? (e.g., what about multiplying microseconds by 1.0/86400000000.0?)

If we do think it's worth handling all of these cases, we probably want a different construct than the equivalent of a lookup table to do this computation.

aaron.ballman added inline comments.Nov 12 2018, 4:02 PM
clang-tidy/abseil/DurationFactoryScaleCheck.cpp
58–59

Hmm, yeah, I suppose it has to! :-D

64

0x0.000001p-126f should get you a new, exciting way to spell 0.

82–85

That's a good point, and part of a broader design discussion: should we support all multipliers?

That's kind of what I'm leaning towards. It's certainly more explainable to users that all the various scaling operations just work, rather than some particular set.

However, maybe you know more about the user base than I do and there's a sound reason to not handle all cases?

If we do think it's worth handling all of these cases, we probably want a different construct than the equivalent of a lookup table to do this computation.

There's a part of me that wonders if we can use std::ratio to describe the scaling operations, but I freely admit I've not thought about implementation strategies all that hard.

hwright updated this revision to Diff 174039.Nov 14 2018, 7:29 AM
hwright marked 11 inline comments as done.

Combined multiplication and division logic, and also now handles scaling of multiple steps (e.g., Seconds * 3600).

clang-tidy/abseil/DurationFactoryScaleCheck.cpp
58–59

I've reworked the bulk of this logic. It still uses doubles, but that doesn't trouble our test cases. Please let me know if there's more to do here.

64

I've added this as a test, and it resolves as normal. Was your comment intended to indicate that it should, or that doing so would be a bug?

82–85

std::ratio didn't look like it made much sense here (though I don't have much first-hand experience using it), but we do now handle multiple scaling steps.

aaron.ballman added inline comments.Nov 14 2018, 10:51 AM
clang-tidy/abseil/DurationFactoryScaleCheck.cpp
64

Hmm, in hindsight, I think this code is okay as-is. The situation I was worried about is where compile-time evaluation gives you one value (say, 0) while runtime evaluation gives you another (nonzero) because of the floating-point format for the target architecture. However, 1) I think APFloat does a good job of avoiding that, so we may be fine by default, and 2) I am questioning whether abseil users would be writing those constants anyway.

74

This doesn't seem quite right. 1.0/6000.0 is less than 1.0/60.0 but isn't a Minutes scaling.

hwright marked 5 inline comments as done.Nov 16 2018, 12:59 PM
hwright added inline comments.
clang-tidy/abseil/DurationFactoryScaleCheck.cpp
74

After spending two days chasing this down, it finally occurred to me: we don't actually handle this case. Our (somewhat conservative) matchers only look for literal values, not division expressions like 1.0/60.0.

And for this iteration of the tool, I think I'm fine with that, since I expect those kinds of things to be exceedingly rare, and probably tricky to handle, anyway.

hwright marked 2 inline comments as done.Nov 16 2018, 1:00 PM

I think this is ready to go, please advise on next steps.

aaron.ballman accepted this revision.Nov 16 2018, 1:29 PM

LGTM!

clang-tidy/abseil/DurationFactoryScaleCheck.cpp
74

Oh jeez -- good point on not covering constant expressions from the matcher. If you think this is going to be exceedingly rare, I'm okay with punting until we need it. Thank you for looking into it!

This revision is now accepted and ready to land.Nov 16 2018, 1:29 PM

@aaron.ballman I don't actually have the commit bit, can you commit this, or are we waiting for further review?

@aaron.ballman I don't actually have the commit bit, can you commit this, or are we waiting for further review?

No further review required. I'm happy to commit for you. I'll do it later today or tomorrow, unless someone else gets to it first.

aaron.ballman closed this revision.Nov 18 2018, 8:43 AM

I've commit in r347163. Thank you for the patch!