Closed Bug 815058 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
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.
Tagging bholley for review since he touched TestAppShellSteadyState (one of the two affected test files) last.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #685067 - Flags: review?(bobbyholley+bmo)
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.
(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).
You can't include strings at the top of nsINode, because you will run into internal/external hell.
How about nsStringGlue.h? That should provide SetIsVoid for you either way
Attachment #685067 - Attachment description: trivial fix v1 → Option #1: include nsDOMString.h in the warning-producing CPP files
Attachment #685067 - Flags: review?(bobbyholley+bmo)
(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.)
Per comment 6 - 7, this effectively just includes nsStringGlue.h, and it builds fine in my local build at least.
Attachment #685221 - Flags: review?(bzbarsky)
Attachment #685221 - Flags: review?(bzbarsky)
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.
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.
Attachment #685067 - Attachment is obsolete: true
Attachment #685221 - Attachment is obsolete: true
Attachment #685338 - Flags: review?(Ms2ger)
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
Attachment #685338 - Flags: review?(Ms2ger) → review+
https://hg.mozilla.org/mozilla-central/rev/a5002d796673
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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
Attachment #685757 - Flags: review?(Ms2ger)
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
Attachment #685757 - Flags: review?(Ms2ger) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.