[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() {
----------------
jdoerfert wrote:
> okura wrote:
> > 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.
> > I'm sorry, I noticed your comment after landing this patch.
> 
> Post review comments are not cause for grief. Git is flexible and we can augment and change things even after a commit. So no worries :)
> 
> > TBH, I'm also not sure whether making it the default is appropriate or not.
> 
> I think this is appropriate. We had one APInt DenseMapInfo specialization, now we have two, both using the same technique. If you don't like the default, people can specify a different one, but not making the only used one the default seems unhelpful as people then will reimplement something without knowing there is a version available.
Thank you for giving a comment on this! I will leave this as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85131