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:
> hwright wrote:
> > 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?
> Hmm, yeah, I suppose it has to! :-D
I've reworked the bulk of this logic. It still uses doubles, but that doesn't
trouble our test cases. Please let me know if there's more to do here.
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:63
+ assert(FloatLit != nullptr && "Neither IntLit nor FloatLit set");
+ return FloatLit->getValueAsApproximateDouble();
+}
----------------
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?
================
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:
> hwright wrote:
> > 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.
> > That's a good point, and part of a broader design discussion: should we
> > support all multipliers?
>
> That's kind of what I'm leaning towards. It's certainly more explainable to
> users that all the various scaling operations just work, rather than some
> particular set.
>
> However, maybe you know more about the user base than I do and there's a
> sound reason to not handle all cases?
>
> > 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.
>
> There's a part of me that wonders if we can use `std::ratio` to describe the
> scaling operations, but I freely admit I've not thought about implementation
> strategies all that hard.
`std::ratio` didn't look like it made much sense here (though I don't have much
first-hand experience using it), but we do now handle multiple scaling steps.
https://reviews.llvm.org/D54246
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
