Closed Bug 733186 Opened 12 years ago Closed 12 years ago

Use MOZ_OVERRIDE more aggressively in layout

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18

People

(Reporter: dholbert, Assigned: Six)

References

Details

(Whiteboard: [mentor=dholbert][lang=c++])

Attachments

(3 files, 5 obsolete files)

Patch with last comments and last mozilla-central update
384.21 KB, patch
Details | Diff | Splinter Review
Script to add moz_override in specific folder
17.90 KB, text/plain
Details
Script to launch the add_override script in whole mozilla-central
595 bytes, text/plain
Details 
Filing this bug on adding MOZ_OVERRIDE annotations in layout.
(on methods that override an inherited method, in derived classes)

For reference on MOZ_OVERRIDE, see:
http://whereswalden.com/2011/11/16/introducing-moz_override-to-annotate-virtual-functions-which-override-base-class-virtual-functions/

(Adding [mentor] whiteboard status, in the hopes that a new contributor might want to take this.)
Whiteboard: [mentor=dholbert] → [mentor=dholbert][lang=c++]
Hi,

I did a perl script that add the "MOZ_OVERRIDE" keyword to all virtual and NS_IMETHOD in derived classes. (when you say "in layout", are you talking about the mozilla_central/layout folder?)
But it seems harder to find a way to know if thoses virtual methods are really overrided from the base class...
I had a syntax question BTW,
when a method is const, should the MOZ_OVERRIDE keyword stand before or after the "const" keyword? or as i think it doesnt matter?
I'm new (and french...) and i just want to give a shot to help :)
(In reply to Six from comment #1)
> I did a perl script that add the "MOZ_OVERRIDE" keyword to all virtual and
> NS_IMETHOD in derived classes.

That's great, thanks!

> (when you say "in layout", are you talking
> about the mozilla_central/layout folder?)

Yes.

> But it seems harder to find a way to know if thoses virtual methods are
> really overrided from the base class...

How so?  If the base class's version is virtual, then the derived class's version is overriding it by definition, right?

Also: the "override" keyword should be enforced by GCC 4.7 (based on http://gcc.gnu.org/projects/cxx0x.html ), so you can probably use that to sanity-check your annotations.  (I'll likely start using GCC 4.7 soon, so if you can't install it for some reason, I'd be happy to try out your patch and see if it blows up.)

> I had a syntax question BTW,
> when a method is const, should the MOZ_OVERRIDE keyword stand before or
> after the "const" keyword? or as i think it doesnt matter?

Good question! It should be _after_ const, like "void foo() const MOZ_OVERRIDE;".  That way, the function signature "void foo() const" is still intact/obvious (as distinguished from a possible non-const version of the function, "void foo()"

(Also -- for future reference, you could answer that by using "mxr.mozilla.org" to search our code for existing examples.  See e.g. these code searches:
  http://mxr.mozilla.org/mozilla-central/search?string=const%20MOZ_OVERRIDE
   --> 13 results
  http://mxr.mozilla.org/mozilla-central/search?string=MOZ_OVERRIDE%20const
   --> 0 results
:)  )
(In reply to Daniel Holbert [:dholbert] from comment #2)

> That's great, thanks!
> Yes.

Nice didn't loose my time :)

> If the base class's version is virtual, then the derived class's
> version is overriding it by definition, right?

Ok, but the difficulty is to know if the base class method is defined as virtual, the script just check if we are in a derivated class and if the method in the derivated class is virtual...
ie:

class A
{
 void toto() const;
};

class B : public A
{
 virtual void tmp();
};

with my script it gives :

class B : public A
{
 virtual void tmp() MOZ_OVERRIDE;
};

As B is derivated from A and toto is virtual in B
it adds MOZ_OVERRIDE to B::toto
that's all.
is it really what you want?

I also take care to place the MOZ_OVERRIDE as :
virtual void toto() MOZ_OVERRIDE { int a = 0; }
 
> Also: the "override" keyword should be enforced by GCC 4.7 (based on
> http://gcc.gnu.org/projects/cxx0x.html ), so you can probably use that to
> sanity-check your annotations.  (I'll likely start using GCC 4.7 soon, so if
> you can't install it for some reason, I'd be happy to try out your patch and
> see if it blows up.)

Ok i'm gonna test it


> Good question! It should be _after_ const, like "void foo() const
> MOZ_OVERRIDE;".  That way, the function signature "void foo() const" is
> still intact/obvious (as distinguished from a possible non-const version of
> the function, "void foo()"

Ok so the script is doing it well.

> (Also -- for future reference, you could answer that by using
> "mxr.mozilla.org" to search our code for existing examples.  See e.g. these
> code searches:
>   http://mxr.mozilla.org/mozilla-central/search?string=const%20MOZ_OVERRIDE
>    --> 13 results
>   http://mxr.mozilla.org/mozilla-central/search?string=MOZ_OVERRIDE%20const
>    --> 0 results
> :)  )

Thanks i keep that in mind.

I'm not at work anymore (did it there) so i'm not gonna be able to test the compilation before lunday

Just a little question that i got now:

If we have a virtual pure method it shouldn't make sense to add MOZ_OVERRIDE to the definition as it doesnt really override the method?
virtual void toto() MOZ_OVERRIDE = 0; //non-sense?

Thank you for your time and explanations :)
(In reply to Six from comment #3)
> As B is derivated from A and toto is virtual in B
> it adds MOZ_OVERRIDE to B::toto
> that's all.
> is it really what you want?

(I'm assuming that "tmp()" and "toto() const" were supposed to be the same function in your example)

That's not quite what I'm looking for.

I believe we want to add MOZ_OVERRIDE to the derived class whenever the following two things are true:
 1. toto() is declared as virtual in the base class (A)
 2. toto() is declared at all in derived class (B), *regardless* of whether that implementation is virtual or not.

Basically: In any situation where B has a declaration of toto() and we expect ((A*)(someConcreteInstanceOfB))->toto() to invoke B::toto(), we want to label B's toto() declaration as MOZ_OVERRIDE.

(At the moment, I don't have any suggestions for how to do this with a perl script -- I'm just clarifying what the desired result is. :) )

(In reply to Six from comment #3)
> If we have a virtual pure method it shouldn't make sense to add MOZ_OVERRIDE
> to the definition as it doesnt really override the method?
> virtual void toto() MOZ_OVERRIDE = 0; //non-sense?

I disagree -- I think that would make sense, *if* that toto() declaration actually does override a method from a base class.

I doubt this situation ever crops up in our code, but I think a pure-virtual MOZ_OVERRIDE would be handy in a situation like this:

  // "grandfather" class
  class A {
    virtual void foo() { doSomething() };
  };

  // "parent" class
  // NOTE: B doesn't implement foo() itself, but it wants to make sure its
  // derived classes provide their own implementation -- they aren't allowed
  // to inherit A's implementation.
  class B : public A {
    virtual void foo() MOZ_OVERRIDE = 0;
  };

  // "child" class
  class C : public B {
    void foo() MOZ_OVERRIDE { printf("hello from C\n"); }
  };

Now -- suppose someone were to come along and change the A::foo() implementation to return "int" instead of "void", and they forgot to change the B and C implementations.  We normally wouldn't catch this bug, UNLESS we had that MOZ_OVERRIDE annotation inside of B.  So, this is why we want that MOZ_OVERRIDE annotation on B there.

> Thank you for your time and explanations :)

No problem, thank you for your work on this!
(Also: feel free to ask questions on irc.mozilla.org #developers! I'm "dholbert" there.  There's a reasonable web interface at http://irc.lc/mozilla/developers/ if you don't have a standalone IRC client configured -- just pick a nickname and click the button, and you'll be in.)
(In reply to Daniel Holbert [:dholbert] from comment #4)
> (In reply to Six from comment #3)
> > As B is derivated from A and toto is virtual in B
> > it adds MOZ_OVERRIDE to B::toto
> > that's all.
> > is it really what you want?
> 
> (I'm assuming that "tmp()" and "toto() const" were supposed to be the same
> function in your example)

Nope it was on purpose becaus this is actually what the script does.
And as a clearly understand your demand i know it should not do this.

> I believe we want to add MOZ_OVERRIDE to the derived class whenever the
> following two things are true:
>  1. toto() is declared as virtual in the base class (A)
>  2. toto() is declared at all in derived class (B), *regardless* of whether
> that implementation is virtual or not.

Ok i got it
dont know already how to do it but i'm gonna try


> > virtual void toto() MOZ_OVERRIDE = 0; //non-sense?

> I disagree -- I think that would make sense, *if* that toto() declaration
> actually does override a method from a base class.

Ok the script does it.

I now "just" have to find a way to find if the base class has some virtual functions that are overrided as virtual in the derivated class.

Again another question:
In this case :
class A {
    virtual void foo() { doSomething() };
  };
//B is not overriding A::foo
  class B : public A {
    void BAR();
  };
//C is overiding A::foo
  class C : public B {
    void foo() MOZ_OVERRIDE { printf("hello from C\n"); }
  };

we need to add MOZ_OVERRIDE on C::foo yeah?
So we have to get the whole heritage tree from C to A.

>(At the moment, I don't have any suggestions for how to do this with a perl >script -- I'm just clarifying what the desired result is. :) )

Don't worry i will work on it, your clarifications really helped me

(In reply to Daniel Holbert [:dholbert] from comment #5)
>Also: feel free to ask questions on irc.mozilla.org #developers!

I will do when i will be back working on it 
Thanks :)
(In reply to Six from comment #6)
> we need to add MOZ_OVERRIDE on C::foo yeah?
> So we have to get the whole heritage tree from C to A.

Correct, yes.

> Don't worry i will work on it, your clarifications really helped me

Great! Thanks, I'm glad that I could help. :)
Hi,

i'm still working on it
doing it in python with the CppHeaderParser module :
http://pydoc.net/CppHeaderParser/latest/

I am actually able to get a list of all classes in layout with their heritage, member fonctions/methods and filename (to modify it later)

There is just a litle problem in the CppHeaderParser module
in this kind of class decl with one or more define around the class name:

class NS_STACK_CLASS Foo MOZ_FINAL
{
/*
class Foo
*/
};

it doesn't get me the right class name wich is Foo
it gives me NS_STACK_CLASS or MOZ_OVERRIDE
i patched it for this use in order to get the real name and it's ok
but it still have problem with derived classes that are not getting...

because i think thoses kind of keyword should be ignored
It's working!
i added 326 MOZ_OVERRIDE in layout/
so in the end there is 365 MOZ_OVERRIDE in this code (mine plus existing)
the compilation worked as expected
i need to add a duplicate function in the script to have correct final statistics
i'm gonna do it tomorrow
Arnaud, if you can post a patch here, I can push it to the Tryserver for you to see if any issues crop up. Also, give the link below a read to make sure that the patch you create has all the needed checkin information included.
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Attached patch First patch to test on TryServer (obsolete) — — Splinter Review 

    
    

    
  
Looking awful green for a first patch! :)

    
    

    
  
Okay great :)

but as i said to daniel we just add MOZ_OVERRIDE everytime when it is totally sure. so there should not be any big bugs by applying it

im gonna continue to work on my script to find other ways to catch more cases where MOZ_OVERRIDE is needed, because the title say : use MOZ_OVERRIDE "more aggressively"

is there somewhere a doc that describes the tbpl interface?

    
    

    
  
Not that I know of. This is a Help dropdown in the upper-left corner that shows most of the options. If you have any other questions, feel free to shoot me an email.

    
    

    
  
Quick update:

At this day there is 628 MOZ_OVERRIDE added by the script for a total of 667
(old stats where 326/365)
just fixed two things in the script to get thoses results.
The compilation works completely

Now i need to find a good way to catch things like this :

class A
{
virtual void Foo(int a);
};

class B : public A
{
void Foo(int);
};

as there is no name for int parameter in B::Foo, 
i cant get a match between A::Foo and B::Foo to add MOZ_OVERRIDE on B::Foo the same way i do right now
i have an idea but im gonna need few days to implement and test it

    
    

    
  
Awesome!

If it's easier, feel free to post several separate patches for the various difficulties-of-detection, rather than trying to do it all in one patch.

    
    

    
  
(In reply to Arnaud Sourioux [:Six] from comment #16)

> Now i need to find a good way to catch things like this :
> 
> class A
> {
> virtual void Foo(int a);
> };
> 
> class B : public A
> {
> void Foo(int);
> };
> 
> as there is no name for int parameter in B::Foo, 
> i cant get a match between A::Foo and B::Foo to add MOZ_OVERRIDE on B::Foo
> the same way i do right now
> i have an idea but im gonna need few days to implement and test it

It is done


So now the script detects 1034 methods that are needing MOZ_OVERRIDE
and adds them all correctly
so we are now at 1073 MOZ_OVERRIDE in layout/

im finalizing another compilation on another computer to be sure
and gonna attach the patch for a TryServer when build ends

I will add a preprocess sanity-check (only on my files) to be sure that moz_override is defined everytime i add it

Thanks,

    
    

    
  


  

        Attachment #659419 -
        Attachment is obsolete: true

    
    

    
  
As MOZ_OVERRIDE is declared this way:

#if defined(MOZ_HAVE_CXX11_OVERRIDE)
#  define MOZ_OVERRIDE          override
#else
#  define MOZ_OVERRIDE          /* no support */
#endif

I did this sanity check :

#if MOZ_OVERRIDE == 0
#endif

Because if MOZ_OVERRIDE is defined by the "else" statement (so its not supported),
g++ will complain about no member on left of operator '==' (MOZ_OVERIDE value is empty)

i added it at the end of every file on wich i add MOZ_OVERRIDE 
and the build went totally fine

this test is of course not included in the patch

Is it possible to give it a new TryServer pass to check that everything is ok?

Im not gonna be able to do more on this bug (except if i find a bug in my script) because it looks like there is no other methods that needs MOZ_OVERRIDE in layout

The next step would be to develop a tool able to find child methods that should override parents methods but not having the exact same signature due to mistake
This is much more complicated that what i did.

Im thinking about it but i cant assure to do it

I want to specially thank Senex for his CppHeaderParser module
https://bitbucket.org/senex/cppheaderparser/

    
    

    
  
Looks like this is busted on a few platforms in BasicTableLayoutStrategy

    
    

    
  
Yes i saw it

MOZ_OVERRIDE isnt defined on:
Armv7a ICS
OS X 10.7 
Android

on thoses platforms, it looks like the file mbft/Atributes.h isnt included.
This file is used to define needed macro to use MOZ_OVERRIDED depending on if the compiler version supports it:

http://mxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#57

As MOZ_OVERRIDE isnt replaced in the logs, it shows that the preprocessor was unable to find it

i guess it is not normal that this file was not included, but i dont know yet why it was not...

    
    

    
  
Ok i guess i found it

many of important files in layout/ are including Attributes.h (often some Interfaces or first class in heritage tree)

so on thoses specific platforms it looks like for a final child class the header inclusion tree is not the same...

a very simple solution would be to add #include "mozilla/Attributes.h" on every file i modify (if it is not already present)

or to find why on thoses platforms this header is not included
but i cant test this last solution i dont have any of thoses platforms at disposition.

    
    

    
  
(In reply to Arnaud Sourioux [:Six] from comment #24)
> a very simple solution would be to add #include "mozilla/Attributes.h" on
> every file i modify (if it is not already present)

That sounds good to me!

    
    

    
  
Ok i did it
i just added the #include to files where i add MOZ_OVERRIDE if this #include isnt already present
i launch the build test on my computer to chec kthat everything is fine
if its ok i will resubmit a patch again

even if it is not directly link to my work i think it would be easier to include it in a generic herder that must be included everywhere, if there is at least one header that is really included in all classes

    
    

    
  
Its should work
as Daniel said it wasn't my fault for the last patch
Attributes.h isn't included everywhere we need it so now this patch does it

Need a new pass on TryServer please :)

    
    

    
  
Comment on attachment 660502 [details] [diff] [review]
Adds 1073 MOZ_OVERRIDE and adds #include "mozilla/Attributes.h" to modified Files (needed for Arm, Android and OS X)

>diff --git a/layout/base/nsDisplayList.h b/layout/base/nsDisplayList.h
[...]
>   virtual bool IsUniform(nsDisplayListBuilder* aBuilder, nscolor* aColor)
>-  {
>+  MOZ_OVERRIDE {

nit: For spots like this, I don't think we want
 "MOZ_OVERRIDE {"
all on its own line, indenting the "{" by a strange amount.  That makes the function a bit messier / a bit harder to read.

Where possible, MOZ_OVERRIDE should go immediately after the close-paren of the function signature.  If it's possible to write your script to follow that rule of thumb, that'd be great!


Also: To request review, you should pick someone (i.e. me) to actually tag for review, in the box next to "review?".  An empty "review?" flag usually won't get a response, since it doesn't tag anyone to explicitly respond to it. :))

(In this case, since the patch is so huge and touches code that I haven't directly worked with, I'll likely flag someone for additional review after I sign off on it -- but you can request review from me as a first pass.)

    
    

    
  
Also, just so you know -- we try our best to keep lines shorter than 80 characters:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style?redirectlocale=en-US&redirectslug=Mozilla_Coding_Style_Guide#Line_Length

I suspect your patch pushes a lot of lines over 80 characters, so those lines would ideally benefit from a newline somewhere. (e.g. before "nscolor" in the chunk of code that I quoted in my previous comment).

I suspect that's a tricky rule to follow for an automatically-generated patch of this magnitude, though, so I probably wouldn't worry about it too much in this bug. (We can probably fix these lines in followups and/or on a case-by-case basis.)

    
    

    
  
Okay 

I agree with you it is just a case that the script doesnt implement yet, i will add a check to see if the '{' is the first important pattern of the line to past the keyword on the previous line.

I will patch it after this TryServer pass in any case this one wouldnt be good enough.

About the review field, i apologise again, i wanted someone to review it but forgot to assign somebody and didnt recheck it before upload...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Adds 1069 MOZ_OVERRIDE (obsolete)
        — 
          — Splinter Review
      

    


  
Fix: always apply MOZ_OVERRIDE on the method line declaration

        Attachment #660502 -
        Attachment is obsolete: true

        Attachment #660502 -
        Flags: review?

        Attachment #660746 -
        Flags: review?(dholbert)

    
    

    
  
NOTE: on "mozilla-inbound"[1], where we'll likely land this patch eventually, there's at least one changeset that slightly bitrots your patch (though your patch does still apply with the command "patch -p1 -F8 < yourpatch.txt").

The bitrotting changeset added one new declaration (for nsDisplayBackground::GetPerFrameKey()) in contextual code, and incidentally, that method itself could use a MOZ_OVERRIDE annotation.

Here's the relevant chunk of that changeset, for reference:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/61b79c82bbc7#l4.43

It isn't particularly surprising that some of this code is changing out from under you, and it probably won't be the only such case to crop up between now & when this gets checked in. Once we're ready to check in, we might want to just generate an up-to-date patch by running your script on mozilla-inbound tip.  Then we can diff that new patch against the version that I'm reviewing, as a sanity-check, and then check it in assuming the only changes are minor & sensible.

[1] mozilla-inbound is the more volatile, quickly-changing repo where we land most things. (and then we merge with mozilla-central every 1-2 days)

    
    

    
  
Ok so now i just have to give you my script?
i will work on it to make it more comprehensive for anyone
and when its done im gonna push it here

thanks

    
    

    
  
Comment on attachment 660746 [details] [diff] [review]
Adds 1069 MOZ_OVERRIDE

># HG changeset patch
># Parent e80ec1d9b1b1299bb2739042eb3811733e936cb4
># User Arnaud Sourioux <six.dsn@gmail.com>
>Bug 733186: Applies 1069 MOZ_OVERRIDE in mozilla-central/layout folder

I'd prefer a commit message like this:
   Bug 733186: Annotate ~1000 methods with MOZ_OVERRIDE in /layout.

Two notes on that change:
(a) It looks like there are actually only 1030 (not 1069) new MOZ_OVERRIDEs, based on
    grep "^+.*MOZ_OVERRIDE" patch.txt  | wc -l
That gives me 1030. That number will likely change (plus or minus a few) before this actually lands, too -- so "around 1000" should cover your bases. :)

(b) Our commit messages generally use active verbs ("Fix XXX"/"Change YYY") rather descriptive ("Fixes XXX"/"Changes YYY"), so "Apply" would be better than "Applies".  That's still a little vague though, so "Annotate N methods with MOZ_OVERRIDE" is even a bit clearer still.

REVIEWING THE CODE:
* As a sanity-check, I removed MOZ_OVERRIDE from all added lines in the patch (all lines that started with "+") and then I applied the result and qrefreshed.  That applied cleanly, and the only remaining changes were additions of #include "mozilla/Attributes.h".

SO: This confirmed that all of the changes were either
 (i) adding " MOZ_OVERRIDE" to a line (and making no other changes to that line)
 ...or...
 (ii) adding the #include

That's great.

Beyond that, I skimmed the patch (didn't read it in its entirety), and I checked a few stylistic things:
 - "const" always comes before MOZ_OVERRIDE (per end of comment 2)
 - In all cases where we have "MOZ_OVERRIDE" and "{" on the same line, the function-signature is also on that line. (so we don't have any instances of comment 29.

The only thing (other than the commit message) that I noticed was: there are 3 lines that have 2 spaces between the ")" and MOZ_OVERRIDE, quoted below:
>+++ b/layout/xul/base/src/nsScrollbarButtonFrame.h
>   NS_IMETHOD HandleMultiplePress(nsPresContext* aPresContext,
>                                  nsGUIEvent *    aEvent,
>                                  nsEventStatus*  aEventStatus,
>-                                 bool aControlHeld)  { return NS_OK; }
>+                                 bool aControlHeld)  MOZ_OVERRIDE { return NS_OK; }

>diff --git a/layout/xul/base/src/nsSliderFrame.h b/layout/xul/base/src/nsSliderFrame.h
>--- a/layout/xul/base/src/nsSliderFrame.h
>+++ b/layout/xul/base/src/nsSliderFrame.h
>   NS_IMETHOD HandleMultiplePress(nsPresContext* aPresContext,
>                                  nsGUIEvent *    aEvent,
>                                  nsEventStatus*  aEventStatus,
>-                                 bool aControlHeld)  { return NS_OK; }
>+                                 bool aControlHeld)  MOZ_OVERRIDE { return NS_OK; }
> 
>   NS_IMETHOD HandleDrag(nsPresContext* aPresContext,
>                         nsGUIEvent *    aEvent,
>-                        nsEventStatus*  aEventStatus)  { return NS_OK; }
>+                        nsEventStatus*  aEventStatus)  MOZ_OVERRIDE { return NS_OK; }
> 

Ideally we should just have 1 space there.  It looks like this happened because there were already 2 spaces between the ")" and the "{".

Rather than having you edit your script to fix these, I'll just push a whitespace-only fix to tweak those lines, so the next patch you generate won't stumble on this.  So, don't worry about this (but we should do a quick sanity-check for this on your final patch when we're ready to land.)

So:  r=me, assuming this still builds on TryServer (which I'll double-check).  Requesting additional-r=dbaron, since he should be in the loop on a change this large to our layout code.

        Attachment #660746 -
        Flags: review?(dholbert)

        Attachment #660746 -
        Flags: review?(dbaron)

        Attachment #660746 -
        Flags: review+

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #36)
> Rather than having you edit your script to fix these, I'll just push a
> whitespace-only fix to tweak those lines, so the next patch you generate
> won't stumble on this.  So, don't worry about this (but we should do a quick
> sanity-check for this on your final patch when we're ready to land.)

Here's the whitespace-only fix:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a92a4356cd8e

And here's a try run of the latest patch applied to mozilla-central:
  https://tbpl.mozilla.org/?tree=Try&rev=c7e85d091dd3

(In reply to Arnaud Sourioux [:Six] from comment #35)
> Ok so now i just have to give you my script?

Yup -- it'd be great if you could post your script here, ideally before we check this in.

That would give us an additional layer of sanity/correctness-checking, and it's also possible we might notice something that your script isn't catching (cases where it could apply MOZ_OVERRIDE but doesn't yet) (though that probably would still be fine, because we can always fix stragglers later on).
Whiteboard: [mentor=dholbert][lang=c++] → [mentor=dholbert][lang=c++][leave open]

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #36)
> (b) Our commit messages generally use active verbs ("Fix XXX"/"Change YYY")
> rather descriptive ("Fixes XXX"/"Changes YYY"), so "Apply" would be better
> than "Applies".  That's still a little vague though, so "Annotate N methods
> with MOZ_OVERRIDE" is even a bit clearer still.

ok got it (trying to improve my way to write commit messages but still not good enough ;) )

> REVIEWING THE CODE:
>  - "const" always comes before MOZ_OVERRIDE (per end of comment 2)
>  - In all cases where we have "MOZ_OVERRIDE" and "{" on the same line, the
> function-signature is also on that line. (so we don't have any instances of
> comment 29.

I'm pretty sure its in the old patch not in the last one.
I cant find any line like this "MOZ_OVERRIDE const"...
I already patch that when you told me and i cant find thoses errors in it.

for the second point you are saying about thoses kind of things?:

//This was the point on comment 29

-  virtual nsRect GetScrollRange() const 
   {
     return mInner.GetScrollRange();
   }
//previous patch did:
+  virtual nsRect GetScrollRange() const 
   MOZ_OVERRIDE {
     return mInner.GetScrollRange();
   }

//This patch does:
+  virtual nsRect GetScrollRange() const MOZ_OVERRIDE 
   {
     return mInner.GetScrollRange();
   }


//One other case that appends
-  virtual nsRect GetScrollRange() const {
+  virtual nsRect GetScrollRange() const MOZ_OVERRIDE {
     return mInner.GetScrollRange();
   }

I don't see what's wrong.

> The only thing (other than the commit message) that I noticed was: there are
> 3 lines that have 2 spaces between the ")" and MOZ_OVERRIDE, quoted below:
> >+++ b/layout/xul/base/src/nsScrollbarButtonFrame.h
> >   NS_IMETHOD HandleMultiplePress(nsPresContext* aPresContext,
> >                                  nsGUIEvent *    aEvent,
> >                                  nsEventStatus*  aEventStatus,
> >-                                 bool aControlHeld)  { return NS_OK; }
> >+                                 bool aControlHeld)  MOZ_OVERRIDE { return NS_OK; }
> 
> >diff --git a/layout/xul/base/src/nsSliderFrame.h b/layout/xul/base/src/nsSliderFrame.h
> >--- a/layout/xul/base/src/nsSliderFrame.h
> >+++ b/layout/xul/base/src/nsSliderFrame.h
> >   NS_IMETHOD HandleMultiplePress(nsPresContext* aPresContext,
> >                                  nsGUIEvent *    aEvent,
> >                                  nsEventStatus*  aEventStatus,
> >-                                 bool aControlHeld)  { return NS_OK; }
> >+                                 bool aControlHeld)  MOZ_OVERRIDE { return NS_OK; }
> > 
> >   NS_IMETHOD HandleDrag(nsPresContext* aPresContext,
> >                         nsGUIEvent *    aEvent,
> >-                        nsEventStatus*  aEventStatus)  { return NS_OK; }
> >+                        nsEventStatus*  aEventStatus)  MOZ_OVERRIDE { return NS_OK; }
> > 
> 
> Ideally we should just have 1 space there.  It looks like this happened
> because there were already 2 spaces between the ")" and the "{".

Yep but thoses spaces do not come from my script and as you said earlier the script only adds MOZ_OVERRIDE and/or the include, i don't want to add things like this in the script to avoid any kind of side effect or special cases, this is why we choosed together to not include the 80_lines_overflow_part in this script. 

> Rather than having you edit your script to fix these, I'll just push a
> whitespace-only fix to tweak those lines, so the next patch you generate
> won't stumble on this.  So, don't worry about this (but we should do a quick
> sanity-check for this on your final patch when we're ready to land.)

ok i will use it if we need another patch thanks :)

    
    

    
  
(In reply to Arnaud Sourioux [:Six] from comment #38)
> (In reply to Daniel Holbert [:dholbert] from comment #36)
> > (b) Our commit messages generally use active verbs ("Fix XXX"/"Change YYY")
> > rather descriptive ("Fixes XXX"/"Changes YYY"), so "Apply" would be better
> > than "Applies".  That's still a little vague though, so "Annotate N methods
> > with MOZ_OVERRIDE" is even a bit clearer still.
> 
> ok got it (trying to improve my way to write commit messages but still not
> good enough ;) )

Thanks!
 
> I'm pretty sure its in the old patch not in the last one.
> I cant find any line like this "MOZ_OVERRIDE const"...

Sorry, I wasn't clear about that.  There isn't anything of that sort that I could find, either -- I wasn't flagging that as something to fix -- rather, I was just listing the things that I checked for.

> Yep but thoses spaces do not come from my script

Right, yeah -- my point was more that "MOZ_OVERRIDE" is part of the function signature, so it'd be marginally more correct for it to hug the signature (the ")") rather than to hug the implementation (the "{"), when there are bogus extra spaces between the signature and implementation.

Anyway, not worth worrying about, now that I removed the bogus spaces on mozilla-central.

So, this looks great! Once dbaron signs off on this, it'll be good to go -- at that point, you'll want to re-generate the patch on top of mozilla-inbound (to fix bitrot from comment 34) and update the commit message (and include "r=dholbert r=dbaron" at the end of the commit message, too).

    
    

    
  
> Anyway, not worth worrying about, now that I removed the bogus spaces on mozilla-central.
(er, "on mozilla-inbound" -- and that'll make it to mozilla-central in the next day or so.)

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #39)
> Sorry, I wasn't clear about that.  There isn't anything of that sort that I
> could find, either -- I wasn't flagging that as something to fix -- rather,
> I was just listing the things that I checked for.

My mistake :)

> Right, yeah -- my point was more that "MOZ_OVERRIDE" is part of the function
> signature, so it'd be marginally more correct for it to hug the signature
> (the ")") rather than to hug the implementation (the "{"), when there are
> bogus extra spaces between the signature and implementation.
> 
> Anyway, not worth worrying about, now that I removed the bogus spaces on
> mozilla-central.
> 

Ok i understand what you mean now

> you'll want to re-generate the patch on top of
> mozilla-inbound (to fix bitrot from comment 34) and update the commit
> message (and include "r=dholbert r=dbaron" at the end of the commit message,
> too).

Will do tomorrow morning :)

    
    

    
  
Comment on attachment 660746 [details] [diff] [review]
Adds 1069 MOZ_OVERRIDE

r=dbaron with dholbert's commit message in comment 36

I'm assuming that you've tested this in a configuration where having MOZ_OVERRIDE on a method that doesn't override a base class method causes a compilation error.

I reviewed this using the commands:

vs 'https://bug733186.bugzilla.mozilla.org/attachment.cgi?id=660746' | sort | less -S
(looking almost entirely at the + lines, to make sure the style/placement made sense)

interdiff -U 6 /dev/null <(vs 'https://bug733186.bugzilla.mozilla.org/attachment.cgi?id=660746' | sed 's/ MOZ_OVERRIDE//') | less
(which showed only additions of #include lines)

        Attachment #660746 -
        Flags: review?(dbaron) → review+

    
    

    
  
OK -- so, Six/Arnaud: if you could generate an up-to-date-patch against your updated mozilla-central tree (or against mozilla-inbound, but probably don't bother with that*), I can sanity-check that against the r+'d version here and land it in the morning!

* Either mozilla-central or mozilla-inbound should be fine, as they were just merged, and it's probably easiest to stick with your existing mozilla-central clone. (after you update it with hg pull -u)

    
    

    
  


  
I checked all things we talk about(doubles spaces, GetPerFrameKey), it ok

        Attachment #660746 -
        Attachment is obsolete: true

    
    

    
  
Looks good to me too!

(I just removed a newline in commit message, before "r=dholbert r=dbaron", since some of our infrastructure hides everything after the first line.)

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2307541f08fb

Thanks so much for adding all these!
Status: NEW → ASSIGNED
Whiteboard: [mentor=dholbert][lang=c++][leave open] → [mentor=dholbert][lang=c++]

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/2307541f08fb
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

    
    

    
  
Arnaud, this is great! Would your script be reusable outside layout? If so, I would be grateful if you could attach it here.

    
    

    
  
Hi

of course it is usable anywhere
i will attach it on this bug as soon as i will make it more readable for others
it can work all alone by specifying the folder you want
but if you need to modify it or anything else i have to make it more understandable first
and this is one of my prior things to do :)

thank you :)

    
    

    
  
Hi Arnaud!

Just wanted to check on that script -- I think we'd still love to use it to fix up other parts of the code, so if you'd be open to postingit (even if it's not quite perfect/readable), that'd be awesome!

    
    

    
  

      
      
      
      

        Attached file
          
        The script to add MOZ_OVERRIDE (obsolete)
        — 
      

    


  
This is the script i used to generate the patches.
It is a litle commented, not a final version but still working good.
To use it you have to download and install https://bitbucket.org/senex/cppheaderparser/downloads at least version dad8a82 but newer is better.

    
    

    
  
I'm really sorry for the delay, i didnt had time to work on it.
but as you needed it i posted a working version.
i will try to refactor it asap.

    
    

    
  
(In reply to Arnaud Sourioux [:Six] from comment #52)
> I'm really sorry for the delay, i didnt had time to work on it.
> but as you needed it i posted a working version.
> i will try to refactor it asap.

Thanks! No worries about the delay, and there's no huge time-pressure on fixing it up -- I just wanted to be sure we hadn't lost you & the potential for improving the rest of the tree with your script. :)

    
    

    
  
Ok I understand :)
i would like to do more but i have too much work (@job) at this time

    
    

    
  


  
Replace the old one.
Fully working
Need to be used with https://bitbucket.org/senex/cppheaderparser/ rev 3e7069f

        Attachment #693301 -
        Attachment is obsolete: true

    
    

    
  


  
It launches the add_override.py script in the whole mozilla-central repo,
one main folder at a time.
it skips gfx and media folders that the add_override script doesnt support due to python recursion limitations.

    
    

    
  
Here you have the tbpl for the last commit i did on layout/

https://tbpl.mozilla.org/?tree=Try&rev=b0786de7d698&pusher=six.dsn@gmail.com

everything works fine.
and i tried locally ths six.py script on the whole mozilla-central repo (except gfx and media folders) and the compilation worked fine too.
so i guess it's done :)

    
  
Blocks: 870516

    
  
Blocks: 973805

    
  
Blocks: 974687

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: