902062 - ccache clang builds fail everywhere we include XPCMaps.h

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[(no longer active)]

Similar to what was discussed in bug 832788.

../../../../js/xpconnect/src/XPCMaps.h:119:31: error: equality comparison with extraneous parentheses [-Werror,-Wparentheses-equality]
        if (((entry)->keyHash == 0))
             ~~~~~~~~~~~~~~~~~^~~~
../../../../js/xpconnect/src/XPCMaps.h:119:31: note: remove extraneous parentheses around the comparison to silence this warning
        if (((entry)->keyHash == 0))
            ~                 ^   ~
../../../../js/xpconnect/src/XPCMaps.h:119:31: note: use '=' to turn this equality comparison into an assignment
        if (((entry)->keyHash == 0))
                              ^~
                              =
../../../../js/xpconnect/src/XPCMaps.h:189:31: error: equality comparison with extraneous parentheses [-Werror,-Wparentheses-equality]
        if (((entry)->keyHash == 0))
             ~~~~~~~~~~~~~~~~~^~~~
../../../../js/xpconnect/src/XPCMaps.h:189:31: note: remove extraneous parentheses around the comparison to silence this warning
        if (((entry)->keyHash == 0))
            ~                 ^   ~
../../../../js/xpconnect/src/XPCMaps.h:189:31: note: use '=' to turn this equality comparison into an assignment
        if (((entry)->keyHash == 0))
                              ^~
                              =
etc.

Bug 831885 worked around this for the jsdhash API by making that an inline function, but bug 884649 removed jsdhash.h and effectively regressed this workaround.

Can we please use to a different hashtable API there, or back out bug 884649 for now?

Comment 1

[Nicholas Nethercote [inactive]]

I read bug 832788 but don't understand how this breaks ccache.

Anyway, would doing bug 831885 for pldhash.h work?  That would be far easier.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> I read bug 832788 but don't understand how this breaks ccache.

With ccache, the file being compiled already has macros expanded (because ccache uses the .i as the cache key, then just passes it to the compiler)

clang does not warn on the code pattern above _if_ it's the result of a macro.  But it has no way to know that when ccache is involved because it's just given the .i.

> Anyway, would doing bug 831885 for pldhash.h work? 

I would think so, yes.

Comment 3

[Terrence Cole [:terrence]]

If you add CCACHE_CPP2=yes to your environment, ccache will do the right thing here and key on the .cpp, fixing the warnings. At least this works for me for SpiderMonkey builds.

Comment 4

[Boris Zbarsky [:bzbarsky]]

It'll also run cpp twice, which leads to somewhat slower builds.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Convert PL_DHASH_ENTRY_IS_* macros to inline functions.

Ehsan, can you check that this unbreaks ccache?  I don't use it and so don't
know what to look for.

Comment 6

[(no longer active)]

Comment on attachment 786654 [details] [diff] [review]
Convert PL_DHASH_ENTRY_IS_* macros to inline functions.

Review of attachment 786654 [details] [diff] [review]:
-----------------------------------------------------------------

This fixes it, but then my build fails because of the same reason using PR_CLIST_IS_EMPTY in nsCacheEntry.cpp :(

This might be an uphill battle, but let's land this patch for now, cause it's not in NSPR!

Comment 7

[Boris Zbarsky [:bzbarsky]]

And a followup to make nsCacheEntry use LinkedList.  ;)

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd9682f22da

Comment 9

[(no longer active)]

(In reply to Boris Zbarsky (:bz) (vacation Aug 10-19) from comment #7)
> And a followup to make nsCacheEntry use LinkedList.  ;)

Bug 902696.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0dd9682f22da