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

Hyrum Wright via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sat Nov 10 18:03:44 PST 2018

hwright added inline comments.

Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:74
+  case DurationScale::Minutes:
+    if (Multiplier == 60.0)
+      return DurationScale::Hours;
hokein wrote:
> we are comparing with floats, I think we should use something `AlmostEquals(Multiplier, 60.0)`.
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.

Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:202
+  // Don't try and replace things inside of macro definitions.
+  if (InsideMacroDefinition(Result, Call->getSourceRange()))
+    return;
hokein wrote:
> isn't `Call->getExprLoc().isMacroID()` enough?
Thanks!  I've also added tests to verify this.


More information about the cfe-commits mailing list