hokein 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>(
----------------
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.
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:22
+// Potential scales of our inputs.
+enum class DurationScale {
+ Hours,
----------------
nit: wrap it in anonymous namespace?
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:74
+ case DurationScale::Minutes:
+ if (Multiplier == 60.0)
+ return DurationScale::Hours;
----------------
we are comparing with floats, I think we should use something
`AlmostEquals(Multiplier, 60.0)`.
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:202
+ // Don't try and replace things inside of macro definitions.
+ if (InsideMacroDefinition(Result, Call->getSourceRange()))
+ return;
----------------
isn't `Call->getExprLoc().isMacroID()` enough?
================
Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:266
+ if (DivBinOp) {
+ DivBinOp->dumpColor();
+ // For division, we only check the RHS.
----------------
is this for debugging?
https://reviews.llvm.org/D54246
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
