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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:50
+  static const std::unordered_map<DurationScale,
+                                  std::tuple<std::string, std::string>>
+      InverseMap(
----------------
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.


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

https://reviews.llvm.org/D54737