[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check
Hyrum Wright via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 26 18:39:54 PST 2018
hwright added a comment.
Sorry it's taken so long to get all the feedback addressed!
================
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},
----------------
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?
================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:50
+ static const std::unordered_map<DurationScale,
+ std::tuple<std::string, std::string>>
+ InverseMap(
----------------
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.
================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:68
+
+ // We know our map contains all the Scale values, so we can skip the
+ // nonexistence check.
----------------
JonasToth wrote:
> The basis for this "we know" might change in the future if `abs::Duration` does things differently. Is adding stuff allowed in the `absl::` space? (i am not fluent with the guarantees that it gives). You could maybe `assert` that the find is always correct, depending on `absl` guarantees.
`absl::` could add things. In this case, I'm very confident they won't, but I've still added the assert.
================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:108
+
+ // TODO(hwright): Check for another condition:
+ // 1. duration-factory-scale
----------------
JonasToth wrote:
> in LLVM the TODO does not contain a name for the author.
I just removed the TODO (this isn't a required part of the check).
================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:125
+ hasAnyName(
+ "::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
+ "::absl::ToDoubleSeconds", "::absl::ToDoubleMilliseconds",
----------------
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54737/new/
https://reviews.llvm.org/D54737
More information about the cfe-commits
mailing list