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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:36
+GetScaleForFactory(llvm::StringRef FactoryName) {
+  static const auto *ScaleMap =
+      new std::unordered_map<std::string, DurationScale>(
----------------
This will cause memory leaks, so may be unique_ptr should be used to hold pointer? May be LLVM ADT has better container?


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:59
+    return IntLit->getValue().getLimitedValue();
+  } else {
+    assert(FloatLit != nullptr);
----------------
else after return. Same in other places.


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:164
+  }
+  return "unreachable";
+}
----------------
Should be llvm_unreachable().


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:288
+  // We can't do anything with this expression, so return.
+  return;
+}
----------------
Please remove trailing return and comment above. See [[ http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-control-flow.html | readability-redundant-control-flow ]].


================
Comment at: docs/ReleaseNotes.rst:88
+  Checks for cases where arguments to ``absl::Duration`` factory functions are
+  scaled internally and could be changed to a different factory function.  This
+  check also looks for arguements with a zero value and suggests using
----------------
Second statement belongs to documentation.


================
Comment at: docs/clang-tidy/checks/abseil-duration-factory-scale.rst:7
+Checks for cases where arguments to ``absl::Duration`` factory functions are
+scaled internally and could be changed to a different factory function.  This
+check also looks for arguements with a zero value and suggests using
----------------
Please fix double space.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54246