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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
