Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21648002:
Extract pair hash functions so that they can be re-used outside of hash tables (Closed)
|
Created:
7 years, 4 months ago by piman Modified:
7 years, 4 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionExtract pair hash functions so that they can be re-used outside of hash tables
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216339
Patch Set 1 #
Total comments: 3
Patch Set 2 : why specialize when you can overload? #Patch Set 3 : use unbiased constant fix #Patch Set 4 : more overloading #Patch Set 5 : MSVC: Y U NO resolve namespace? #Patch Set 6 : move HashPair implementation out of gcc ifdef #Patch Set 7 : Fix RenderPass::ID hash #Messages
Total messages: 13 (0 generated)
For https://codereview.chromium.org/20667002/ I need to hash 2 ints together. I figured best is to extract the hash function here. I added a bit of template magic to avoid duplicating the hashing code. danakj: please review the general idea and the constant change. ajwong: please review the template magic. brettw: OWNERs https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h File base/containers/hash_tables.h (right): https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h... base/containers/hash_tables.h:84: uint64 odd_random = 481046412LL << 32 | 1025306953LL; note: I changed the value here so that it is, actually, odd.
LGTM for making these into functions, if we can make this compile correctly on windows. https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h File base/containers/hash_tables.h (right): https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h... base/containers/hash_tables.h:84: uint64 odd_random = 481046412LL << 32 | 1025306953LL; On 2013/08/01 23:14:44, piman wrote: > note: I changed the value here so that it is, actually, odd. pedantic nit: as you pointed out the other day, 55 instead of 53 would be more correct in not changing the bits that were randomly generated. https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h... base/containers/hash_tables.h:252: template<typename Type1, typename Type2> I see you are having fun with the windows compiler here as well. I think this needs to be of the form template<> and youll need to use all those DEFINE macros to declare instances of this function. But perhaps awong@ will have some more concrete advice. I know I had an epic battle getting this stuff to compile on windows.
On Thu, Aug 1, 2013 at 7:10 PM, <danakj@chromium.org> wrote: > LGTM for making these into functions, if we can make this compile > correctly on > windows. > > > > https://codereview.chromium.**org/21648002/diff/1/base/** > containers/hash_tables.h<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h> > File base/containers/hash_tables.h (right): > > https://codereview.chromium.**org/21648002/diff/1/base/** > containers/hash_tables.h#**newcode84<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h#newcode84> > base/containers/hash_tables.h:**84: uint64 odd_random = 481046412LL << 32 > | 1025306953LL; > On 2013/08/01 23:14:44, piman wrote: > >> note: I changed the value here so that it is, actually, odd. >> > > pedantic nit: as you pointed out the other day, 55 instead of 53 would > be more correct in not changing the bits that were randomly generated. > > https://codereview.chromium.**org/21648002/diff/1/base/** > containers/hash_tables.h#**newcode252<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h#newcode252> > base/containers/hash_tables.h:**252: template<typename Type1, typename > Type2> > I see you are having fun with the windows compiler here as well. I think > this needs to be of the form template<> and youll need to use all those > DEFINE macros to declare instances of this function. But perhaps awong@ > will have some more concrete advice. I know I had an epic battle getting > this stuff to compile on windows. > Yeah, annoying, you can't do partial specialization of functions. > > https://codereview.chromium.**org/21648002/<https://codereview.chromium.org/2... >
On Thu, Aug 1, 2013 at 8:05 PM, Antoine Labour <piman@chromium.org> wrote: > > > > On Thu, Aug 1, 2013 at 7:10 PM, <danakj@chromium.org> wrote: > >> LGTM for making these into functions, if we can make this compile >> correctly on >> windows. >> >> >> >> https://codereview.chromium.**org/21648002/diff/1/base/** >> containers/hash_tables.h<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h> >> File base/containers/hash_tables.h (right): >> >> https://codereview.chromium.**org/21648002/diff/1/base/** >> containers/hash_tables.h#**newcode84<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h#newcode84> >> base/containers/hash_tables.h:**84: uint64 odd_random = 481046412LL << 32 >> | 1025306953LL; >> On 2013/08/01 23:14:44, piman wrote: >> >>> note: I changed the value here so that it is, actually, odd. >>> >> >> pedantic nit: as you pointed out the other day, 55 instead of 53 would >> be more correct in not changing the bits that were randomly generated. >> >> https://codereview.chromium.**org/21648002/diff/1/base/** >> containers/hash_tables.h#**newcode252<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h#newcode252> >> base/containers/hash_tables.h:**252: template<typename Type1, typename >> Type2> >> I see you are having fun with the windows compiler here as well. I think >> this needs to be of the form template<> and youll need to use all those >> DEFINE macros to declare instances of this function. But perhaps awong@ >> will have some more concrete advice. I know I had an epic battle getting >> this stuff to compile on windows. >> > > Yeah, annoying, you can't do partial specialization of functions. > Overloading seems to work though, so I did that. > >> >> https://codereview.chromium.**org/21648002/<https://codereview.chromium.org/2... >> > >
On 2013/08/02 03:41:40, piman wrote: > On Thu, Aug 1, 2013 at 8:05 PM, Antoine Labour <mailto:piman@chromium.org> wrote: > > > > > > > > > On Thu, Aug 1, 2013 at 7:10 PM, <mailto:danakj@chromium.org> wrote: > > > >> LGTM for making these into functions, if we can make this compile > >> correctly on > >> windows. > >> > >> > >> > >> https://codereview.chromium.**org/21648002/diff/1/base/** > >> > containers/hash_tables.h<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h> > >> File base/containers/hash_tables.h (right): > >> > >> https://codereview.chromium.**org/21648002/diff/1/base/** > >> > containers/hash_tables.h#**newcode84<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h#newcode84> > >> base/containers/hash_tables.h:**84: uint64 odd_random = 481046412LL << 32 > >> | 1025306953LL; > >> On 2013/08/01 23:14:44, piman wrote: > >> > >>> note: I changed the value here so that it is, actually, odd. > >>> > >> > >> pedantic nit: as you pointed out the other day, 55 instead of 53 would > >> be more correct in not changing the bits that were randomly generated. > >> > >> https://codereview.chromium.**org/21648002/diff/1/base/** > >> > containers/hash_tables.h#**newcode252<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h#newcode252> > >> base/containers/hash_tables.h:**252: template<typename Type1, typename > >> Type2> > >> I see you are having fun with the windows compiler here as well. I think > >> this needs to be of the form template<> and youll need to use all those > >> DEFINE macros to declare instances of this function. But perhaps awong@ > >> will have some more concrete advice. I know I had an epic battle getting > >> this stuff to compile on windows. > >> > > > > Yeah, annoying, you can't do partial specialization of functions. > > > > Overloading seems to work though, so I did that. Will read over in detail tomorrow, but on this one comment, the general workaround is to use a struct with a static method and partially specialize the struct. If you need type inference, you front that with a function that does the inference so it can explicitly instantiate the correct struct. This is the relation between Bind() and BindState. > > > > > >> > >> > https://codereview.chromium.**org/21648002/%3Chttps://codereview.chromium.org...> > >> > > > >
On 2013/08/02 03:41:40, piman wrote: > On Thu, Aug 1, 2013 at 8:05 PM, Antoine Labour <mailto:piman@chromium.org> wrote: > > > > > > > > > On Thu, Aug 1, 2013 at 7:10 PM, <mailto:danakj@chromium.org> wrote: > > > >> LGTM for making these into functions, if we can make this compile > >> correctly on > >> windows. > >> > >> > >> > >> https://codereview.chromium.**org/21648002/diff/1/base/** > >> > containers/hash_tables.h<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h> > >> File base/containers/hash_tables.h (right): > >> > >> https://codereview.chromium.**org/21648002/diff/1/base/** > >> > containers/hash_tables.h#**newcode84<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h#newcode84> > >> base/containers/hash_tables.h:**84: uint64 odd_random = 481046412LL << 32 > >> | 1025306953LL; > >> On 2013/08/01 23:14:44, piman wrote: > >> > >>> note: I changed the value here so that it is, actually, odd. > >>> > >> > >> pedantic nit: as you pointed out the other day, 55 instead of 53 would > >> be more correct in not changing the bits that were randomly generated. > >> > >> https://codereview.chromium.**org/21648002/diff/1/base/** > >> > containers/hash_tables.h#**newcode252<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h#newcode252> > >> base/containers/hash_tables.h:**252: template<typename Type1, typename > >> Type2> > >> I see you are having fun with the windows compiler here as well. I think > >> this needs to be of the form template<> and youll need to use all those > >> DEFINE macros to declare instances of this function. But perhaps awong@ > >> will have some more concrete advice. I know I had an epic battle getting > >> this stuff to compile on windows. > >> > > > > Yeah, annoying, you can't do partial specialization of functions. > > > > Overloading seems to work though, so I did that. Will read over in detail tomorrow, but on this one comment, the general workaround is to use a struct with a static method and partially specialize the struct. If you need type inference, you front that with a function that does the inference so it can explicitly instantiate the correct struct. This is the relation between Bind() and BindState. > > > > > >> > >> > https://codereview.chromium.**org/21648002/%3Chttps://codereview.chromium.org...> > >> > > > >
On Thu, Aug 1, 2013 at 10:37 PM, <ajwong@chromium.org> wrote: > On 2013/08/02 03:41:40, piman wrote: > >> On Thu, Aug 1, 2013 at 8:05 PM, Antoine Labour <mailto:piman@chromium.org >> > >> > wrote: > > > >> > >> > >> > On Thu, Aug 1, 2013 at 7:10 PM, <mailto:danakj@chromium.org> wrote: >> > >> >> LGTM for making these into functions, if we can make this compile >> >> correctly on >> >> windows. >> >> >> >> >> >> >> >> https://codereview.chromium.****org/21648002/diff/1/base/** >> >> >> > > containers/hash_tables.h<https**://codereview.chromium.org/** > 21648002/diff/1/base/**containers/hash_tables.h<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h> > > > >> >> File base/containers/hash_tables.h (right): >> >> >> >> https://codereview.chromium.****org/21648002/diff/1/base/** >> >> >> > > containers/hash_tables.h#****newcode84<https://codereview.** > chromium.org/21648002/diff/1/**base/containers/hash_tables.h#**newcode84<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h#newcode84> > > > >> >> base/containers/hash_tables.h:****84: uint64 odd_random = 481046412LL >> << 32 >> >> >> | 1025306953LL; >> >> On 2013/08/01 23:14:44, piman wrote: >> >> >> >>> note: I changed the value here so that it is, actually, odd. >> >>> >> >> >> >> pedantic nit: as you pointed out the other day, 55 instead of 53 would >> >> be more correct in not changing the bits that were randomly generated. >> >> >> >> https://codereview.chromium.****org/21648002/diff/1/base/** >> >> >> > > containers/hash_tables.h#****newcode252<https://codereview.** > chromium.org/21648002/diff/1/**base/containers/hash_tables.h#**newcode252<https://codereview.chromium.org/21648002/diff/1/base/containers/hash_tables.h#newcode252> > > > >> >> base/containers/hash_tables.h:****252: template<typename Type1, >> typename >> >> >> Type2> >> >> I see you are having fun with the windows compiler here as well. I >> think >> >> this needs to be of the form template<> and youll need to use all those >> >> DEFINE macros to declare instances of this function. But perhaps awong@ >> >> will have some more concrete advice. I know I had an epic battle >> getting >> >> this stuff to compile on windows. >> >> >> > >> > Yeah, annoying, you can't do partial specialization of functions. >> > >> > > Overloading seems to work though, so I did that. >> > > Will read over in detail tomorrow, but on this one comment, the general > workaround is to use a struct with a static method and partially > specialize the > struct. If you need type inference, you front that with a function that > does > the inference so it can explicitly instantiate the correct struct. This > is the > relation between Bind() and BindState. > In this case I don't get to choose, what has to participate in the overload resolution is the function stdext::hash_values 'cause that's what the MSVC hash_map looks for. > > > > > >> >> >> >> >> > > https://codereview.chromium.****org/21648002/%3Chttps://codere** > view.chromium.org/21648002/ <http://codereview.chromium.org/21648002/>> > >> >> >> > >> > >> > > > > https://codereview.chromium.**org/21648002/<https://codereview.chromium.org/2... >
LGTM
Ok, finally got it working on MSVC, sigh. danakj: PTAL, I also had to fix the hash_value implementation for RenderPass::Id, it uses overloading rather than specialization, turns out, same as all other implementations for other types that I could find in chrome. brettw: still need an OWNER stamp.
Thanks for the heroics to get this working, LGTM
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/piman@chromium.org/21648002/34001
Message was sent while issue was closed.
Change committed as 216339 |
