[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:63
+  assert(FloatLit != nullptr && "Neither IntLit nor FloatLit set");
+  return FloatLit->getValueAsApproximateDouble();
+}
----------------
hwright wrote:
> aaron.ballman wrote:
> > 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`.
> 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?
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.


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:73
+  case DurationScale::Hours:
+    if (Multiplier <= 1.0 / 60.0)
+      return std::make_tuple(DurationScale::Minutes, Multiplier * 60.0);
----------------
This doesn't seem quite right. `1.0/6000.0` is less than `1.0/60.0` but isn't a `Minutes` scaling.


https://reviews.llvm.org/D54246