hwright marked 12 inline comments as done.
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},
----------------
aaron.ballman wrote:
> sammccall wrote:
> > aaron.ballman wrote:
> > > sammccall wrote:
> > > > aaron.ballman wrote:
> > > > > 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.
> > > > There are three main issues with global objects that the coding
> > > > standard mentions:
> > > > - performance when unused. Not relevant to function-local statics,
> > > > only constructed when used
> > > > - static initialization order fiasco (constructors). Not relevant to
> > > > function-local statics, constructed in well-defined order
> > > > - static initialization order fiasco (destructors). Just as relevant
> > > > to function-local statics as to any other object!
> > > >
> > > > That's why I say it violates the rule in spirit: the destructor is
> > > > global. https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2
> > > >
> > > > > I don't see any destructor ordering issues with this
> > > > The reason these constructs are disallowed in coding guidelines is that
> > > > humans can't see the problems, and they manifest in non-local ways :-(
> > > >
> > > > > leaking would be less than ideal
> > > > Why? The current code will (usually) destroy the object when the
> > > > program exits. This is a waste of CPU, the OS will free the memory.
> > > > The reason these constructs are disallowed in coding guidelines is that
> > > > humans can't see the problems, and they manifest in non-local ways :-(
> > >
> > > Can you explain why you think this would have any non-local impact on
> > > destruction? The DenseMap is local to the function and a pointer to it
> > > never escapes (so nothing can rely on it), and the data contained are
> > > StringRefs wrapping string literals and integer values.
> > >
> > > > Why? The current code will (usually) destroy the object when the
> > > > program exits. This is a waste of CPU, the OS will free the memory.
> > >
> > > Static analysis tools will start barking about memory leaks which then
> > > adds noise to the output, hiding real issues.
> > > Can you explain why you think this would have any non-local impact on
> > > destruction?
> > I haven't analyzed this case in detail. A trivial example is running this
> > check on a detached thread (sure, don't do that, but that's a non-local
> > constraint...). It's possible there are no others here.
> >
> > > Static analysis tools will start barking about memory leaks which then
> > > adds noise to the output, hiding real issues.
> >
> > This is a common pattern (from C++ FAQ) that tools are IME aware of. The
> > object is still reachable so it's not truly a leak. (The heap checkers I
> > know of are dynamic, but this is even easier to recognize statically).
> >
> > (@hwright: sorry for the derail here, I know you're aware of this issue)
> I guess I still read the coding standard differently. While it's certainly
> possible to have a finalization order fiasco with local statics, I believe
> you have to work considerably harder to craft one. I was doing some code
> archaeology on the words in the coding standard, and I don't think this was
> the scenario we had in mind when crafting that rule, but these words have
> been around for quite some time and I may have missed contextual discussion
> from the mailing lists.
>
> It might make sense to propose a patch that adds some clarity to the coding
> standard in this area. We use static locals like this in numerous places
> (IdentifierNamingCheck.cpp, TypePromotionInMathFnCheck.cpp,
> TypeMismatchCheck.cpp, etc) and it would be good to nail down what we want to
> have happen in that case.
>
> As for this code review, I still think using a heap allocation is not a good
> approach. If there is a serious concern about finalization ordering, we can
> use a `ManagedStatic` instead.
From all this discussion, it sounds like leaving the heap allocation as-is is
what we want here.
Though I also understand an am sad about the potential for finalization
ordering, and I'm doubly sad that C++ doesn't give us a good way to create a
static map like this which doesn't require //any// finalization.
================
Comment at: clang-tidy/abseil/DurationComparisonCheck.cpp:50
+ static const std::unordered_map<DurationScale,
+ std::tuple<std::string, std::string>>
+ InverseMap(
----------------
JonasToth wrote:
> hwright wrote:
> > 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)))
> > ```
> in order to use `DenseMap` you need to specialize the `DenseMapInfo` for
> types, that are not supported yet. More in the llvm manual and by examples.
> Storing the integer is no issue, but the `enum class` makes its a type on its
> own, same with the pair.
>
> I believe its fine to leave it an `unordered_map`, even though it might be
> slower on access. You can certainly do the extra-mile.
I'll just leave this as-in, since it looks like `DenseMapInfo` wants unused
sentinel values, which I'm loathe to add to the enum.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54737/new/
https://reviews.llvm.org/D54737
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
