1127511 - 144 lines of MSVC build warning output from "warning C4396: 'js::EmptyShape::ensureInitialCustomShape' : the inline specifier cannot be used when a friend declaration refers to a specialization of a function template"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Holbert [:dholbert]]

Noticed a bunch of instances of this build warning when skimming through a recent Win32 build log:
{
Warning: C4396 in $SRC\js\src\vm\RegExpObject.h: 'js::EmptyShape::ensureInitialCustomShape' : the inline specifier cannot be used when a friend declaration refers to a specialization of a function template
$SRC\js\src\vm/RegExpObject.h(452) : warning C4396: 'js::EmptyShape::ensureInitialCustomShape' : the inline specifier cannot be used when a friend declaration refers to a specialization of a function template

Warning: C4396 in $SRC\js\src\vm\ErrorObject.h: 'js::EmptyShape::ensureInitialCustomShape' : the inline specifier cannot be used when a friend declaration refers to a specialization of a function template
$SRC\js\src\vm/ErrorObject.h(40) : warning C4396: 'js::EmptyShape::ensureInitialCustomShape' : the inline specifier cannot be used when a friend declaration refers to a specialization of a function template

Warning: C4396 in $SRC\js\src\vm\StringObject.h: 'js::EmptyShape::ensureInitialCustomShape' : the inline specifier cannot be used when a friend declaration refers to a specialization of a function template
$SRC\js\src\vm/StringObject.h(65) : warning C4396: 'js::EmptyShape::ensureInitialCustomShape' : the inline specifier cannot be used when a friend declaration refers to a specialization of a function template
}

Overall, this issue triggers 144 lines of warning-spam, in this latest m-c build win32 log:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32-debug/1422544309/mozilla-central-win32-debug-bm82-build1-build774.txt.gz

(based on grepping for "ensureInitialCustomShape' : the inline specifier ")

Looks like this function (ensureInitialCustomShape) was added in bug 724768.

Comment 1

[Daniel Holbert [:dholbert]]

This templated function is defined (as "inline", per the warning-message) here:
http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Shape-inl.h?rev=7a7a248c492f#133

...and specializations of it are declared as "friend" functions in (at least) these three places:
http://mxr.mozilla.org/mozilla-central/source/js/src/vm/ErrorObject.h?rev=7a7a248c492f#38
http://mxr.mozilla.org/mozilla-central/source/js/src/vm/RegExpObject.h?rev=7a7a248c492f#450
http://mxr.mozilla.org/mozilla-central/source/js/src/vm/StringObject.h?rev=7a7a248c492f#63

And these headers are presumably #included all over the place, which is why this is so spammy.

Comment 2

[Daniel Holbert [:dholbert]]

This build warning is documented here:
  https://msdn.microsoft.com/en-us/library/bb384968%28v=vs.90%29.aspx
That page says MSVC just ignores "inline" in this case.  So, should we drop the "inline" keyword, since it's not having any effect (at least on MSVC)?  (and perhaps we should then also move the function out of its "-inl" file)

Or is there something else we should do here?

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

We probably should not remove the "inline" here if it is added by design. I wrote some code to test the behavior of different compilers, and it seems that the "inline" here doesn't affect the asm code generated by GCC, but does affect the code by Clang.

But I wonder whether we can directly make |assignInitialShape| of the objects public, so that we can get rid of the friend declartions here. Alternatively, we can just move the |assignInitialShape|s out from there classes to an independent function template without adding any additional friend. AFAICS, all of those functions only call method NativeObject::addDataProperty() which is public. I see no reason why we cannot expose them to the public.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8567712 [details] [diff] [review]
patch

Sorry, I've kinda lost touch with this code.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af0936a4d98f

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38c14390bd27

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/38c14390bd27