[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:25
+static llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
+  static const std::unordered_map<std::string, DurationScale> ScaleMap(
+      {{"ToDoubleHours", DurationScale::Hours},
----------------
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)


================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:125
+                  hasAnyName(
+                      "::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
+                      "::absl::ToDoubleSeconds", "::absl::ToDoubleMilliseconds",
----------------
hwright wrote:
> JonasToth wrote:
> > the list here is somewhat duplicated with the static members in the functions at the top. it would be best to merge them.
> > Not sure on how much `constexpr` is supported by the llvm-datastructures, but a constexpr `DenseMap.keys()` would be nice. Did you try something along this line?
> I haven't tried that exact formulation, but given the above issue with `DenseMap`, I'm not sure it will work.  Happy to try once we get it ironed out.
> 
> Another thing I've thought about is factoring the `functionDecl` matcher into a separate function, because I expect it to be reused.  I haven't been able to deduce what type that function would return.
the type is probably `clang::ast_matchers::internal::Matcher<FunctionDecl>`. Not sure what the `bind` does with the type though.

If that is not the case you can make a nice error with `int Bad = functionDecl(anything());`, the type should be printed somewhere ;)
You could maybe create a lambda for that matcher or an `AST_MATCHER(...)` which some checks do as well. All variants are in use with clang-tidy.


================
Comment at: clang-tidy/abseil/DurationRewriter.h:20
+// Duration factory and conversion scales
+enum class DurationScale {
+  Hours,
----------------
You could specify the underlying type to `std::int8` directly, making the enum smaller and a more space-efficient `DenseMap`.


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

https://reviews.llvm.org/D54737