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