[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;
> 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()))
> isn't `Call->getExprLoc().isMacroID()` enough?
Thanks! I've also added tests to verify this.
More information about the cfe-commits