525094 - [HTML5] nsHtml5* classes don't follow the mozilla naming conventions

Status: NEW

(Core :: DOM: HTML Parser, defect, P4)

Description

[(dormant account)]

In particular struct members do not follow the mName format which means that there is a lot of member shadowing going on which prevents us from deploying static analysis in bug 522776

Comment 1

[Henri Sivonen (:hsivonen)]

The code is automatically translated from Java. The original follows the Java naming conventions. When I was writing the Java to C++ translator, I asked people if I should put cycles into making the translator munge names into the Mozilla format, and I was told that I shouldn't bother.

It seems there's now a reason to add the name munging.

Comment 2

[Benjamin Smedberg]

I don't think we particularly care about mozilla-style naming in auto-generated code. We *do* care that local variables don't shadow class-scope variables: that's a recipe for confusion and getting the wrong variable, especially in machine-generated code.

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to comment #2)
> I don't think we particularly care about mozilla-style naming in auto-generated
> code. We *do* care that local variables don't shadow class-scope variables:
> that's a recipe for confusion and getting the wrong variable, especially in
> machine-generated code.

Does this also apply method arguments when the field is this-qualified?

There's a lot of:
public void setFoo(bar foo) {
  this.foo = foo;
}

Comment 4

[Benjamin Smedberg]

Yes, I think that's a dangerous pattern. Probably the easiest solution is to qualify all members with an m_ prefix, or qualify all locals with a simple underscore prefix.

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to comment #4)
> Yes, I think that's a dangerous pattern. Probably the easiest solution is to
> qualify all members with an m_ prefix,

That would have to go in the translator, because prefixing fields is Eclipse-poison in Java. If the Java side needs to change, I'd rather have newFoo arguments than m_foo fields.

Comment 6

[Benjamin Smedberg]

Yes, I meant the translator. I have no knowledge or opinion about the Java code (although it seems like member shadowing is bad form there too, no?)

Comment 7

[(dormant account)]

(In reply to comment #3)
> (In reply to comment #2)
> > I don't think we particularly care about mozilla-style naming in auto-generated
> > code. We *do* care that local variables don't shadow class-scope variables:
> > that's a recipe for confusion and getting the wrong variable, especially in
> > machine-generated code.
> 
> Does this also apply method arguments when the field is this-qualified?
> 
> There's a lot of:
> public void setFoo(bar foo) {
>   this.foo = foo;
> }

I think this is a reasonable pattern to allow. I special-cased the shadow-member checker to allow parameters as it's a really common pattern.

Ie if we do flag this kind o fcode, I'd rather downgrade this particular pattern to a warning instead of an error.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> > There's a lot of:
> > public void setFoo(bar foo) {
> >   this.foo = foo;
> > }
> 
> I think this is a reasonable pattern to allow. I special-cased the
> shadow-member checker to allow parameters as it's a really common pattern.
> 
> Ie if we do flag this kind o fcode, I'd rather downgrade this particular
> pattern to a warning instead of an error.

Agreed, and btw SpiderMonkey C++ style guide now says no to mFoo, instead using this->foo if there's any shadowing. Many C++ experts (hi, Luke) use this style, if not _ or m_ or other mangling. Any convention is unreliable, so it's not as if mFoo somehow is "safer" than this->foo.

/be

Comment 9

[(dormant account)]

(In reply to comment #8)
> (In reply to comment #7)
> > > There's a lot of:
> > > public void setFoo(bar foo) {
> > >   this.foo = foo;
> > > }
> > 
> > I think this is a reasonable pattern to allow. I special-cased the
> > shadow-member checker to allow parameters as it's a really common pattern.
> > 
> > Ie if we do flag this kind o fcode, I'd rather downgrade this particular
> > pattern to a warning instead of an error.
> 
> Agreed, and btw SpiderMonkey C++ style guide now says no to mFoo, instead using
> this->foo if there's any shadowing. Many C++ experts (hi, Luke) use this style,
> if not _ or m_ or other mangling. Any convention is unreliable, so it's not as
> if mFoo somehow is "safer" than this->foo.

It is safer in that following moz naming convention makes it impossible to get into a shadowing problem(ie if one accidentally avoids this->). Furthermore, gcc doesn't tell us if this->(as it's just syntax) was used or not, so our tools can't check that. Luckily spidermonkey code didn't have any problems with local variables shadowing, so we can still make it an error(once this bug is fixed).

Comment 10

[Luke Wagner [:luke]]

(In reply to comment #9)
> Luckily spidermonkey code didn't have any problems with
> local variables shadowing, so we can still make it an error(once this bug is
> fixed).

Not "luckily"; more like, we can have our cake (no shadowing) and eat it too (no mandatory uglification) :)

Comment 11

[Brendan Eich [:brendan]]

True, leaving off m by accident *and* lower-casing the then-leading letter would be required, but people do forget (Gecko has cases of arguments not named aBar and IIRC members not named mFoo). My point is conventions rely on fallible human beans and that's not "safe".

Great to have static analysis help, including flexibility to accomodate several patterns.

/be

Comment 12

[Luke Wagner [:luke]]

Out of curiosity, is there a special case that ignores shadowing in constructors?  I think the pattern:

class A {
  int x;
  A(int x) : x(x) {}
};

is fairly common and would set off alarms without exemption.

Comment 13

[Henri Sivonen (:hsivonen)]

Does http://hg.mozilla.org/mozilla-central/rev/805ee8a86251 address the concerns?

I addressed all Java warnings from Eclipse, including member hiding warnings. However, the pattern
setFoo(Bar foo) {
  this.foo = foo;
}
or the same pattern in a constructor is typical in Java and not a warning there, so I didn't remove it.

Considering the comments about the spidermonkey style above, is it OK to leave this setter/constructor pattern in?

Comment 14

[Henri Sivonen (:hsivonen)]

Attachment: Pushed patch