980317 - Turn nsIContentHandle into a class that asserts its nsIContent* vs. nsIContent** state

Status: ASSIGNED

(Core :: DOM: HTML Parser, defect)

Description

[Henri Sivonen (:hsivonen)]

Follow-up to bug 959150 per bug 959150 comment 21.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Change nsIContentHandle into a class

Let's see if this compiles on all our compilers:
https://tbpl.mozilla.org/?tree=Try&rev=41ef892d41f9

Comment 2

[Henri Sivonen (:hsivonen)]

Fail:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35725009&tree=Try&full=1#error0

Does b2g emulator have an older compiler than our other targets?

Comment 3

[Henri Sivonen (:hsivonen)]

Filed bug 980824.

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Change nsIContentHandle into a class, v2

https://tbpl.mozilla.org/?tree=Try&rev=db167bfdbf13

Comment 5

[Henri Sivonen (:hsivonen)]

https://tbpl.mozilla.org/?tree=Try&rev=120e00231364

Comment 6

[Henri Sivonen (:hsivonen)]

https://tbpl.mozilla.org/?tree=Try&rev=ccd482ade004

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Change nsIContentHandle into a class, v3

Comment 8

[Henri Sivonen (:hsivonen)]

Looks like the compiler for B2G ICS emulator is just too broken:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35791355&tree=Try&full=1

smaug, I don't see a way to work around this in nsIContentHandle.h. I think we shouldn't start making changes outside nsIContentHandle.h to cater to broken compilers. Any ideas other than waiting for a build system hacker to deal with bug 980824?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Don't use ?: , but if-else

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #9)
> Don't use ?: , but if-else

Well, yeah, but I meant avoiding changes like that when I said "I think we shouldn't start making changes outside nsIContentHandle.h to cater to broken compilers."

I'll try to find out how hard it would be to get the compiler situation fixed, since it's so weird that B2G ICS is worse than B2G JB or Android ICS.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8387564 [details] [diff] [review]
Change nsIContentHandle into a class, v3

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

I have gcc45 on my system -- easy to do a test build with the changes below, might as well, will report back with successfulness of this approach.  I'm fairly confident, this is not unusual compared to the other stuff I encountered when introducing NullptrT (or when using it in not-yet-landed patches), but it's extremely easy to make thinkos here.

True nullptr cannot come soon enough.

::: parser/html/nsIContentHandle.h
@@ +57,5 @@
> +#endif
> +  {
> +  }
> +
> +  nsIContentHandle(mozilla::NullptrT aNull)

You're not going to be able to rely on nullptr having a distinct type here, so long as gcc 4.4/4.5 exist.  You can make this get used if nullptr *does* have a distinct type, like so:

template<typename T>
nsIContentHandle(N,
                 typename mozilla::EnableIf<mozilla::IsNullPointer<N>::value,
                                            int>::Type dummy = 0)
{
  ...
}

Modulo typos above.

@@ +67,5 @@
> +#endif
> +  }
> +
> +  // Alternative to the above constructor to work around bug 980824.
> +  nsIContentHandle(int aNull)

This shouldn't be added.  If you want precise mState checking, have a DEBUG conditional in the nsIContent* overload.

@@ +144,5 @@
> +    return *this;
> +  }
> +
> +  // Alternative to the above operator to work around bug 980824.
> +  nsIContentHandle& operator=(int aNull)

This shouldn't be added, __null will select operator=(mozilla::NullptrT), I believe.

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Extra diff to fix some of the nullptr compat issues

So, I think this largely but not entirely solves the match-nullptr issues.  (The nullptr-or-int-or-long thing should be wrapped up in an mfbt trait, but that's easily done after the fact -- and js/public/RootingAPI.h will want such a thing too.))

The one-ish remaining issue after that is that

  cond ? nullptr : nsIContentHandle()

doesn't compile with emulated nullptr.  I tried to grok the standard language for type conversions, but I suspect it says that if you have a null pointer constant on one arm, and a non-pointer class on the other, the null pointer constant won't convert to the class type, even if the class type has an overload for the null pointer constant type.  I would suggest using nsIContentHandle(nullptr) in all those cases (look to be maybe six or so), but I dunno what you want exactly here, so throwing that aspect into your end of the court.