[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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},
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54737/new/

https://reviews.llvm.org/D54737