hwright added inline comments.
================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:35
+getInverseForScale(DurationScale Scale) {
+ static const std::unordered_map<DurationScale,
+ std::pair<llvm::StringRef, llvm::StringRef>>
----------------
lebedev.ri wrote:
> hwright wrote:
> > lebedev.ri wrote:
> > > https://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options
> > > > We never use hash_set and unordered_set because they are generally very
> > > > expensive (each insertion requires a malloc) and very non-portable.
> > >
> > > Since the key is an enum, [[
> > > https://llvm.org/docs/ProgrammersManual.html#llvm-adt-indexedmap-h |
> > > `llvm/ADT/IndexedMap.h` ]] should be a much better fit.
> > It doesn't look like `IndexedMap` has a constructor which takes an
> > initializer list. Without it, this code get a bit more difficult to read,
> > and I'd prefer to optimize for readability here.
> The manual still 'recommends' not to use them.
> Simple solution: immediately invoked lambda
> Better solution: try to add such constructor to `IndexedMap`.
In hopes of not making this too much of a yak shave, I've gone with the
immediately invoked lambda.
================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:74
+ Node, *Result.Context))) {
+ return tooling::fixit::getText(*MaybeCallArg, *Result.Context).str();
+ }
----------------
lebedev.ri wrote:
> hwright wrote:
> > lebedev.ri wrote:
> > > So you generate fix-it, and then immediately degrade it into a string.
> > > Weird.
> > This doesn't generate a fix-it, it just fetches the text of the given node
> > as a `StringRef` but we're returning a `string`, so we need to convert.
> >
> > Is there a more canonical method I should use to fetch a node's text?
> I don't know the answer, but have you tried looking what
> `tooling::fixit::getText()` does internally?
I have. It calls `internal::getText`, which, from the namespace, I'm hesitant
to call in this context.
================
Comment at: clang-tidy/abseil/DurationRewriter.cpp:156
+llvm::Optional<DurationScale> getScaleForInverse(llvm::StringRef Name) {
+ static const llvm::DenseMap<llvm::StringRef, DurationScale> ScaleMap(
+ {{"ToDoubleHours", DurationScale::Hours},
----------------
lebedev.ri wrote:
> Are you very sure this shouldn't be [[
> https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringmap-h |
> `StringMap` ]]?
Nope. Thanks for the catch!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55245/new/
https://reviews.llvm.org/D55245
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
