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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Nov 11 14:45:30 PST 2018


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





More information about the cfe-commits mailing list