sammccall added inline comments.
================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:25
+static llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
+ static const std::unordered_map<std::string, DurationScale> ScaleMap(
+ {{"ToDoubleHours", DurationScale::Hours},
----------------
aaron.ballman wrote:
> sammccall wrote:
> > aaron.ballman wrote:
> > > sammccall wrote:
> > > > hwright wrote:
> > > > > JonasToth wrote:
> > > > > > hwright wrote:
> > > > > > > JonasToth wrote:
> > > > > > > > I think this could be made a `DenseMap` with just the right
> > > > > > > > amount of dense entries. 12 elements seems not too much to me.
> > > > > > > > Does the key-type need to be `std::string`, or could it be
> > > > > > > > `StringRef`(or `StringLiteral` making everything `constexpr` if
> > > > > > > > possible)?
> > > > > > > > Is there some strange stuff with dangling pointers or other
> > > > > > > > issues going on?
> > > > > > > Conceptually, this could easily be `constexpr`, but my compiler
> > > > > > > doesn't seem to want to build something which is called `static
> > > > > > > constexpr llvm::DenseMap<llvm::StringRef, DurationScale>`. It's
> > > > > > > chief complaint is that such a type has a non-trivial destructor.
> > > > > > > Am I using this correctly?
> > > > > > I honestly never tried to make a `constexpr DenseMap<>` but it
> > > > > > makes sense it is not possible, as `DenseMap` is involved with
> > > > > > dynamic memory after all, which is not possible with `constexpr`
> > > > > > (yet). So you were my test-bunny ;)
> > > > > >
> > > > > > I did reread the Data-structures section in the LLVM manual, i
> > > > > > totally forgot about `StringMap` that maps strings to values.
> > > > > > `DenseMap` is good when mapping small values to each other, as we
> > > > > > do here (`const char* -> int (whatever the enum deduces too)`),
> > > > > > which would fit.
> > > > > > `StringMap` does allocations for the strings, which we don't need.
> > > > > >
> > > > > > `constexpr` aside my bet is that `DenseMap` fits this case the
> > > > > > better, because we can lay every thing out and never touch it
> > > > > > again. Maybe someone else has better arguments for the decision :)
> > > > > >
> > > > > > Soooooo: could you please try `static const
> > > > > > llvm::DenseMap<llvm::StringRef, DurationScale>`? :)
> > > > > > (should work in theory: https://godbolt.org/z/Qo7Nv4)
> > > > > `static const llvm::DenseMap<llvm::StringRef, DurationScale>` works
> > > > > here. :)
> > > > Sorry for the drive-by...
> > > > This has a non-trivial destructor, so violates
> > > > https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors
> > > > at least in spirit.
> > > >
> > > > Best to leak it (`auto &ScaleMap = *new const
> > > > llvm::DenseMap<...>(...)`) to avoid the destructor issues. The
> > > > constructor issues are already handled by making it function-local.
> > > I do not think this violates the coding standard -- that's talking mostly
> > > about global objects, not local objects, and is concerned about startup
> > > performance and initialization orders. I don't see any destructor
> > > ordering issues with this, so I do not think any of that applies here and
> > > leaking would be less than ideal.
> > There are three main issues with global objects that the coding standard
> > mentions:
> > - performance when unused. Not relevant to function-local statics, only
> > constructed when used
> > - static initialization order fiasco (constructors). Not relevant to
> > function-local statics, constructed in well-defined order
> > - static initialization order fiasco (destructors). Just as relevant to
> > function-local statics as to any other object!
> >
> > That's why I say it violates the rule in spirit: the destructor is global.
> > https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2
> >
> > > I don't see any destructor ordering issues with this
> > The reason these constructs are disallowed in coding guidelines is that
> > humans can't see the problems, and they manifest in non-local ways :-(
> >
> > > leaking would be less than ideal
> > Why? The current code will (usually) destroy the object when the program
> > exits. This is a waste of CPU, the OS will free the memory.
> > The reason these constructs are disallowed in coding guidelines is that
> > humans can't see the problems, and they manifest in non-local ways :-(
>
> Can you explain why you think this would have any non-local impact on
> destruction? The DenseMap is local to the function and a pointer to it never
> escapes (so nothing can rely on it), and the data contained are StringRefs
> wrapping string literals and integer values.
>
> > Why? The current code will (usually) destroy the object when the program
> > exits. This is a waste of CPU, the OS will free the memory.
>
> Static analysis tools will start barking about memory leaks which then adds
> noise to the output, hiding real issues.
> Can you explain why you think this would have any non-local impact on
> destruction?
I haven't analyzed this case in detail. A trivial example is running this check
on a detached thread (sure, don't do that, but that's a non-local
constraint...). It's possible there are no others here.
> Static analysis tools will start barking about memory leaks which then adds
> noise to the output, hiding real issues.
This is a common pattern (from C++ FAQ) that tools are IME aware of. The object
is still reachable so it's not truly a leak. (The heap checkers I know of are
dynamic, but this is even easier to recognize statically).
(@hwright: sorry for the derail here, I know you're aware of this issue)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54737/new/
https://reviews.llvm.org/D54737
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
