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