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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
