aaron.ballman added inline comments.
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:39
+static llvm::Optional<DurationScale>
+GetScaleForFactory(llvm::StringRef FactoryName) {
+ static const std::unordered_map<std::string, DurationScale> ScaleMap(
----------------
GetScaleForFactory -> getScaleForFactory, per naming conventions. Same for the
other functions.
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:48
+
+ const auto ScaleIter = ScaleMap.find(FactoryName);
+ if (ScaleIter == ScaleMap.end())
----------------
Drop top-level `const` (this returns an iterator, not a pointer/reference).
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:55
+
+// Given either an integer or float literal, return it's value.
+// One and only one of `IntLit` and `FloatLit` should be provided.
----------------
it's -> its
================
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) {
----------------
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);`
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:59
+ const FloatingLiteral *FloatLit) {
+ if (IntLit) {
+ return IntLit->getValue().getLimitedValue();
----------------
Elide these braces.
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:63
+ assert(FloatLit != nullptr && "Neither IntLit nor FloatLit set");
+ return FloatLit->getValueAsApproximateDouble();
+}
----------------
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.
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:72
+ case DurationScale::Hours:
+ // We can't scale Hours.
+ break;
----------------
Hours with a multiplier of `1.0/60.0` would scale to minutes, wouldn't it?
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:81-84
+ if (Multiplier == 60.0)
+ return DurationScale::Minutes;
+ if (Multiplier == 1e-3)
+ return DurationScale::Milliseconds;
----------------
What about scaling with a multiplier of 3600 to go from seconds to hours, and
other plausible conversions?
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:112
+// `Divisor` would produce a new scale. If so, return it, otherwise `None`.
+static llvm::Optional<DurationScale> GetNewDivScale(DurationScale OldScale,
+ double Divisor) {
----------------
Similar comments here as above. In fact, I feel like these functions should be
combined into `getNewScale()` and have normalized the scaling value in the
caller so that it covers both operations rather than manually spelling out the
conversions in either direction. e.g., convert everything into multiplier form
in the caller by treating divisors as a multiplication by 1/divisor.
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:150
+// constructing a `Duration` for that scale.
+static std::string GetFactoryForScale(DurationScale Scale) {
+ switch (Scale) {
----------------
This should return a `StringRef`.
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:194
+
+ // Don't try and replace things inside of macro definitions.
+ if (Call->getExprLoc().isMacroID())
----------------
and -> to
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:198
+
+ const Expr *Arg = Call->getArg(0)->IgnoreImpCasts();
+ // Arguments which are macros are ignored.
----------------
I suspect you want to ignore paren expressions as well, so
`IgnoreParenImpCasts()`.
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:213
+ const auto *CallDecl = Result.Nodes.getNodeAs<FunctionDecl>("call_decl");
+ auto MaybeScale = GetScaleForFactory(CallDecl->getName());
+ if (!MaybeScale)
----------------
Do not use `auto` here as the type is not explicitly spelled out in the
initialization.
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.h:19
+
+/// This check finds cases where the incorrect `Duration` factory funciton is
+/// being used by looking for scaling constants inside the factory argument
----------------
funciton -> function
https://reviews.llvm.org/D54246
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
