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},
----------------
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. :)
================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:50
+ static const std::unordered_map<DurationScale,
+ std::tuple<std::string, std::string>>
+ InverseMap(
----------------
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)))
```
================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:125
+ hasAnyName(
+ "::absl::ToDoubleHours", "::absl::ToDoubleMinutes",
+ "::absl::ToDoubleSeconds",
"::absl::ToDoubleMilliseconds",
----------------
JonasToth wrote:
> 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.
I was able to get both the factory matcher and the conversion matcher factored
out. It's still a little messy, since the result has to be wrapped with a
`functionDecl` at the call site, but that how the type system wanted it. (And
it also allows for the `bind` call to work.)
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
