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

[Hyrum Wright via Phabricator via cfe-commits]

hwright marked 12 inline comments as done.
hwright 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:
> > > > 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)
> I guess I still read the coding standard differently. While it's certainly possible to have a finalization order fiasco with local statics, I believe you have to work considerably harder to craft one. I was doing some code archaeology on the words in the coding standard, and I don't think this was the scenario we had in mind when crafting that rule, but these words have been around for quite some time and I may have missed contextual discussion from the mailing lists.
> 
> It might make sense to propose a patch that adds some clarity to the coding standard in this area. We use static locals like this in numerous places (IdentifierNamingCheck.cpp, TypePromotionInMathFnCheck.cpp, TypeMismatchCheck.cpp, etc) and it would be good to nail down what we want to have happen in that case.
> 
> As for this code review, I still think using a heap allocation is not a good approach. If there is a serious concern about finalization ordering, we can use a `ManagedStatic` instead.
>From all this discussion, it sounds like leaving the heap allocation as-is is what we want here.

Though I also understand an am sad about the potential for finalization ordering, and I'm doubly sad that C++ doesn't give us a good way to create a static map like this which doesn't require //any// finalization.


================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:50
+  static const std::unordered_map<DurationScale,
+                                  std::tuple<std::string, std::string>>
+      InverseMap(
----------------
JonasToth wrote:
> hwright wrote:
> > hwright wrote:
> > > JonasToth wrote:
> > > > This variable is a little hard to read. Could you make a little wrapper-struct instead of the `tuple` to make clear which element represents what?
> > > > Otherwise, why not `std::pair`?
> > > > 
> > > > - same `DenseMap` argument
> > > > - same `StringRef`/`StringLiteral` instead `string` consideration
> > > `std::pair` works here.
> > > 
> > > I'll defer the `DenseMap` and `StringRef`/`StringLiteral` changes until we determine if they are actually possible.
> > ...but using `DenseMap` here doesn't work.  From the mountains of compiler output I get when I try, it appears that `DenseMap` adds some constraints on the key type, and an `enum class` doesn't meet those constraints.
> > 
> > fwiw, the errors are mostly of this form:
> > ```
> > /llvm/llvm/include/llvm/ADT/DenseMap.h:1277:45: error: ‘getEmptyKey’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
> >      const KeyT Empty = KeyInfoT::getEmptyKey();
> >                         ~~~~~~~~~~~~~~~~~~~~~^~
> > /llvm/llvm/include/llvm/ADT/DenseMap.h:1278:53: error: ‘getTombstoneKey’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
> >      const KeyT Tombstone = KeyInfoT::getTombstoneKey();
> >                             ~~~~~~~~~~~~~~~~~~~~~~~~~^~
> > /llvm/llvm/include/llvm/ADT/DenseMap.h:1280:44: error: ‘isEqual’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
> >      while (Ptr != End && (KeyInfoT::isEqual(Ptr[-1].getFirst(), Empty) ||
> >                            ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /llvm/llvm/include/llvm/ADT/DenseMap.h:1281:44: error: ‘isEqual’ is not a member of ‘llvm::DenseMapInfo<clang::tidy::abseil::DurationScale>’
> >                            KeyInfoT::isEqual(Ptr[-1].getFirst(), Tombstone)))
> > ```
> in order to use `DenseMap` you need to specialize the `DenseMapInfo` for types, that are not supported yet. More in the llvm manual and by examples.
> Storing the integer is no issue, but the `enum class` makes its a type on its own, same with the pair.
> 
> I believe its fine to leave it an `unordered_map`, even though it might be slower on access. You can certainly do the extra-mile.
I'll just leave this as-in, since it looks like `DenseMapInfo` wants unused sentinel values, which I'm loathe to add to the enum.


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

https://reviews.llvm.org/D54737