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

[Hyrum Wright via Phabricator via cfe-commits]

hwright 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) {
----------------
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?


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:63
+  assert(FloatLit != nullptr && "Neither IntLit nor FloatLit set");
+  return FloatLit->getValueAsApproximateDouble();
+}
----------------
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?


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:81-84
+    if (Multiplier == 60.0)
+      return DurationScale::Minutes;
+    if (Multiplier == 1e-3)
+      return DurationScale::Milliseconds;
----------------
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.


https://reviews.llvm.org/D54246