aaron.ballman 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},
----------------
sammccall wrote:
> hwright wrote:
> > 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. :)
> Sorry for the drive-by...
> This has a non-trivial destructor, so violates
> https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors at
> least in spirit.
>
> Best to leak it (`auto &ScaleMap = *new const llvm::DenseMap<...>(...)`) to
> avoid the destructor issues. The constructor issues are already handled by
> making it function-local.
I do not think this violates the coding standard -- that's talking mostly about
global objects, not local objects, and is concerned about startup performance
and initialization orders. I don't see any destructor ordering issues with
this, so I do not think any of that applies here and leaking would be less than
ideal.
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
