[PATCH] D85131: [NFC][APInt][DenseMapInfo] Move DenseMapAPIntKeyInfo into DenseMap.h as DenseMapInfo<APInt>

[Shinji Okumura via Phabricator via llvm-commits]

okura added inline comments.


================
Comment at: llvm/include/llvm/ADT/DenseMapInfo.h:352
+/// Provide DenseMapInfo for APInt.
+template <> struct DenseMapInfo<APInt> {
+  static inline APInt getEmptyKey() {
----------------
kuter wrote:
> okura wrote:
> > kuter wrote:
> > > It seems that,  it is possible to specify arbitrary key info traits for `DenseSet`. 
> > > This was what `DenseMapAPIntKeyInfo` in `LLVMContextImpl.h` was dong.
> > > It would declare a `DenseSet` like this:
> > > `DenseSet<APInt, DenseMapAPIntKeyInfo>`
> > > 
> > > This specialization   might not be desirable for every use case since `static_cast<unsigned>` discards information. 
> > > 
> > > Maybe we want to do this in the Attributor like the way `LLVMContextImpl.h` was doing it ?
> > Sorry, I didn't understand what you mean. 
> > 
> > > This specialization might not be desirable for every use case since static_cast<unsigned> discards information.
> > This specialization has already been in `LLVMContextImpl.h` and I didn't change anything.
> > 
> > I want to use it in Attributor, but it exists under `/lib` directory and we were not able to include it directly.
> > So I moved it into here.
> What I was saying is that you made this the default for `APInt`.
> I wasn't exactly sure that this should be the default.
> 
> I was talking about keeping it as `DenseMapAPIntKeyInfo`.
> I wrote this before this revision got accepted.
> 
> What I was saying is that you made this the default for APInt.
> I wasn't exactly sure that this should be the default.
I understood the point. I'm sorry for my misunderstanding.

TBH, I'm also not sure whether making it the default is appropriate or not.
(I made this change based on my interpretation that it was implemented as the default.)

Hmm..., I feel we should hear from others on this issue.

> I wrote this before this revision got accepted.
I'm sorry, I noticed your comment after landing this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85131/new/

https://reviews.llvm.org/D85131