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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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>(
----------------
hokein wrote:
> aaron.ballman wrote:
> > hwright wrote:
> > > Eugene.Zelenko wrote:
> > > > This will cause memory leaks, so may be unique_ptr should be used to hold pointer? May be LLVM ADT has better container?
> > > This is a tradeoff between leaking a small amount of known memory for the duration of the program, and constructing this map every time this function is invoked.  Since I expect the latter to occur frequently, that's a tradeoff I think is acceptable.   (Ideally, this would be a compile-time constant, but sadly we don't yet have a `constexpr` dictionary type.)
> > > 
> > > Of course, if there is a more typical way of doing that here, I'm happy to use it.
> > Why did you not use a static value type?
> I think main reason is lazy initialization.
Static locals are lazily initialized, so I'm still not certain why this shouldn't use `static const std::unorderd_map<std::string, DurationScale> ScaleMap{...};` instead.


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:46
+  const auto ScaleIter = ScaleMap->find(FactoryName);
+  if (ScaleIter == ScaleMap->end()) {
+    return llvm::None;
----------------
Elide braces.


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:57
+                       const FloatingLiteral *FloatLit) {
+  if (IntLit) {
+    return IntLit->getValue().getLimitedValue();
----------------
Elide braces.


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:60
+  }
+  assert(FloatLit != nullptr);
+  return FloatLit->getValueAsApproximateDouble();
----------------
Please add a message to the assertion. e.g., `assert(foo && "bar");`


================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:179-181
+              functionDecl(hasAnyName("absl::Nanoseconds", "absl::Microseconds",
+                                      "absl::Milliseconds", "absl::Seconds",
+                                      "absl::Minutes", "absl::Hours"))
----------------
`::absl` instead -- otherwise this can trigger in unintended places.


https://reviews.llvm.org/D54246