Looking at this I remembered that we have

internal static int CombineHashCodes(int h1, int h2, int h3, int h4, int h5)

in Utils.cs.

Well on the one hand it would be nice to deduplicate our code.

On the other hand:

• The original intent of the refactor here was to roll the repeated operations into a loop. CombineHashCodes() doesn't have a params int[] hs overload, so we'd have to unroll the loop again.
• The implementation in CombineHashCodes() still uses XOR. This StackOverflow answer discusses the implementation we borrowed from System.Web.Util.HashCodeCombiner and I notice that another answer is Jon Skeet again disrecommending XOR for hashcodes.

Not for this PR, but maybe it's worth:

• Changing CombineHashCodes() to use the add/multiply method, and
• Adding a CombineHashCodes(int h, param int[] hs) overload

If we did that it would probably then be worth moving the ModuleSpecificationComparer.GetHashCode() method over to use Utils.CombineHashCodes().

I agree about XOR but we use it very frequently - I propose to don't block the PR and to postpone the change to a special PR where cleanup all code base. Please open new tracking Issue if you agree.

For reference. We have also:

internal static int SequenceGetHashCode<T>(this IEnumerable<T> xs)

public int GetHashCode(Tuple<ITypeName, TypeResolutionState> obj)

public override int GetHashCode(ICollection<T> obj)

If the only use of p is to initialize result why not just initialize result directly.

I figured it looks better than just throwing 23 in there (I noticed a few stack overflow answers on this tended to say "I dunno why we used that number", so I thought it was worth calling them p and q). But not attached to it -- can happily change them back to inline constants.

While the loop is nicer code it does mean that you doing an allocation where there wasn't one before.

Yes I tried to look for a better way to do this (without the allocation), but I'm guessing it's not possible. Either we keep the allocation or we unroll the loop. Thought the loop was nicer initially, but can go either way.

I agree that we should avoid such allocations.
As we discussed above we could review all code base to clean up GetHashCode methods to use unified approach.

So everybody put the same questions I suggest now use
internal static int CombineHashCodes(int h1, int h2, int h3, int h4, int h5)
and defer improvements to new PR.

I imagine CombineHashCodes may not get improved for a while if ever and for large numbers of modules, a better distribution is helpful. How about if I unroll the loop but use the new hash algorithm?

Ok.

If we do end up updating the CombineHashCodes method, we should circle back here and call that.