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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
