[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:57-58
+// One and only one of `IntLit` and `FloatLit` should be provided.
+static double GetValue(const IntegerLiteral *IntLit,
+                       const FloatingLiteral *FloatLit) {
+  if (IntLit) {
----------------
hwright wrote:
> aaron.ballman wrote:
> > 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);`
> >  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?
Hmm, yeah, I suppose it has to! :-D


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:63
+  assert(FloatLit != nullptr && "Neither IntLit nor FloatLit set");
+  return FloatLit->getValueAsApproximateDouble();
+}
----------------
hwright wrote:
> aaron.ballman wrote:
> > 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.
> Do you have an example which I could put in a test?
`0x0.000001p-126f` should get you a new, exciting way to spell `0`.


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:81-84
+    if (Multiplier == 60.0)
+      return DurationScale::Minutes;
+    if (Multiplier == 1e-3)
+      return DurationScale::Milliseconds;
----------------
hwright wrote:
> aaron.ballman wrote:
> > What about scaling with a multiplier of 3600 to go from seconds to hours, and other plausible conversions?
> 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.
> 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.


https://reviews.llvm.org/D54246