815058 - nsINode.h: inline function ‘void SetDOMStringToNull(nsAString&)’ used but never defined

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert]]

Build warning encountered today, when my local opt build reaches test_IHistory.cpp:
{
27:07.65 test_IHistory.cpp
27:09.23 In file included from ../../../../../dist/include/nsIDocument.h:20:0,
27:09.23                  from ../../../../../dist/include/nsIContent.h:10,
27:09.23                  from ../../../../../dist/include/mozilla/dom/Link.h:15,
27:09.23                  from /scratch/work/builds/mozilla-central/mozilla-central.12-03-19.10-25/mozilla/toolkit/components/places/tests/cpp/mock_Link.h:14,
27:09.23                  from /scratch/work/builds/mozilla-central/mozilla-central.12-03-19.10-25/mozilla/toolkit/components/places/tests/cpp/test_IHistory.cpp:13:
27:09.23 Warning: enabled by default in /scratch/work/builds/mozilla-central/mozilla-central.12-03-19.10-25/mozilla/../obj/dist/include/nsINode.h: inline function ‘void SetDOMStringToNull(nsAString&)’ used but never defined
27:09.23 ../../../../../dist/include/nsINode.h:63:13: warning: inline function ‘void SetDOMStringToNull(nsAString&)’ used but never defined [enabled by default]
}

Looks like nsINode.h declares an inline helper-function "SetDOMStringToNull", and the definition lives in a completely different header file (in nsDOMString.h).

AFAICT, this function's declaration can just be removed from nsINode.h.  I'm verifying that now in a local opt & debug build.  Setting dependency on bug 780469 since that's what added this declaration.

Comment 1

[Daniel Holbert [:dholbert]]

Oh, I see -- it's declared there because we have some inline code in nsINode.h itself that calls SetDOMStringToNull().  So if I remove the declaration, the build fails.

So instead of removing the declaration, we just need to make sure that any code that #includes nsINode.h also includes nsDOMString.h (directly or indirectly).  Judging by instances of this warning, that's the case everywhere so far, except for two cases:
  widget/tests/TestAppShellSteadyState.cpp
  toolkit/components/places/tests/cpp/test_IHistory.cpp

We can fix this warning by adding #includes in those files.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Option #1: include nsDOMString.h in the warning-producing CPP files

Tagging bholley for review since he touched TestAppShellSteadyState (one of the two affected test files) last.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I don't think this makes sense. Either we include nsDOMString.h in nsINode.h, or we just use SetIsVoid() in nsINode.h, like we do elsewhere.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to :Ms2ger from comment #3)
> I don't think this makes sense. Either we include nsDOMString.h in
> nsINode.h

That's fine by me -- we had it a "#include nsDOMString.h" there at one point, and bug 780469 replaced it with this function decl.  Happy to add it back -- aryeh, would you object to that (since you took it out)?

>  we just use SetIsVoid() in nsINode.h, like we do elsewhere.

Not sure we can do that -- that requires we've got the string functionality #included (instead of just forward-declared) at the top of nsINode.h, and I'm not sure we have that (or want to rely on having that).

Comment 5

[Boris Zbarsky [:bzbarsky]]

You can't include strings at the top of nsINode, because you will run into internal/external hell.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

How about nsStringGlue.h? That should provide SetIsVoid for you either way

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to :Ms2ger from comment #6)
> How about nsStringGlue.h? That should provide SetIsVoid for you either way

(FWIW, that's the only thing #included by nsDOMString.h.  So, #including nsDOMString.h gets us exactly that, plus the SetDOMStringToNull function-definition, so that we won't need to call SetIsVoid directly.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Option #2: include nsDOMString.h in nsINode.h

Per comment 6 - 7, this effectively just includes nsStringGlue.h, and it builds fine in my local build at least.

Comment 9

[Daniel Holbert [:dholbert]]

Actually: nsINode.h already indirectly #includes nsDOMString.h, via nsINodeInfo.h -- but only if MOZILLA_INTERNAL_API is #defined.

> 31 #ifdef MOZILLA_INTERNAL_API
> 32 #include "nsDOMString.h"
> 33 #endif
https://mxr.mozilla.org/mozilla-central/source/content/base/public/nsINodeInfo.h#32

That's why we only get this warning in test CPP files.

So -- it's a bit bogus to be forward-declaring an inline method in a header file, and then calling that method in the same header file, when the impl of that inline method is only available to certain compilation units (those with MOZILLA_INTERNAL_API defined) and not others...  Hmm.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Option #3: Remove SetDOMStringToNull fwd-declaration, and shift usages to nsINode.cpp

Per prev comment, I think we shouldn't be making calls to internally-visible-only APIs in an externally-includable header file.

So, this patch moves these function bodies from the header to the .cpp file, and removes the forward-declaration.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 685338 [details] [diff] [review]
Option #3: Remove SetDOMStringToNull fwd-declaration, and shift usages to nsINode.cpp

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

r=me

Comment 12

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5002d796673

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a5002d796673

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: followup: mark directories as FAIL_ON_WARNINGS

This marks the two directories that this was previously causing warnings in (from comment 1) as FAIL_ON_WARNINGS.

This had a green Try run (w/ just builds for Mac/Linux):
  https://tbpl.mozilla.org/?tree=Try&rev=029dfd5006aa

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 685757 [details] [diff] [review]
followup: mark directories as FAIL_ON_WARNINGS

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

Fine with me

::: toolkit/components/places/tests/cpp/Makefile.in
@@ +10,2 @@
>  
>  relativesrcdir = @relativesrcdir@

Please move relativesrcdir between VPATH and FAIL_ON_WARNINGS

Comment 16

[Daniel Holbert [:dholbert]]

Fixed that & pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2b6cc93b25ff

Thanks!

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b6cc93b25ff