1316290 - Run clang-tidy on layout/ module

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Andi [:andi]]

Using the checkers provided by clang-tidy we can refresh the code to make it use the features of C++11 like:

- auto variables declarations
- default CTORS and DTORS
- using new range loop operators

Comment 1

[Andi [:andi]]

Attachment: Bug 1316290 - Use 'auto' type specifier for some variable decls that are assigned to "new", in /layout.

Review commit: https://reviewboard.mozilla.org/r/91678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91678/

Comment 2

[Andi [:andi]]

Attachment: Bug 1316290 - Replace default bodies of special member functions with = default;

Review commit: https://reviewboard.mozilla.org/r/91680/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91680/

Comment 3

[Andi [:andi]]

Attachment: Bug 1316290 - Use C++11's override and remove already-implied virtual where applicable, in /layout.

Review commit: https://reviewboard.mozilla.org/r/91682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91682/

Comment 4

[Andi [:andi]]

Started a try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cd2e26309ecfaea3501beb3c850d2d322fbab0c

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8808988 [details]
Bug 1316290 - Use 'auto' type specifier for some variable decls that are assigned to "new", in /layout.

https://reviewboard.mozilla.org/r/91678/#review92118

Commit message nit:
> Bug 1316290 - Use auto type specifier for variable declarations to improve code readability and maintainability

This is a bit too vague about the extent of what's changing.  It almost sounds like you're saying that *all* variable declarations are changing here, across the tree. :)  And that is more than we want to actually change, and more than this patch actually does change.

I'd suggest instead...
Bug 1316290 - Use 'auto' type specifier for some variable decls that are assigned to "new", in /layout.
...or something like that.

(If you want to add something about code readability & maintainability, feel free -- but that might make the message too long, so it might be better to shift that out of the commit message's first line at least.)

::: layout/style/nsCSSRuleProcessor.cpp:282
(Diff revision 1)
>  {
>    NS_PRECONDITION(from != to, "This is not going to work!");
>    RuleHashTableEntry *oldEntry =
>      const_cast<RuleHashTableEntry*>(
>        static_cast<const RuleHashTableEntry*>(from));
> -  RuleHashTableEntry *newEntry = new (KnownNotNull, to) RuleHashTableEntry();
> +  auto *newEntry = new (KnownNotNull, to) RuleHashTableEntry();

While you're touching the type here, please also fix it to shift the "*" to the left of the space (so it's "auto* foo" instead of "auto *foo"), to align with our coding style [1].

This applies to every chunk in the patch except for the very first.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8808989 [details]
Bug 1316290 - Replace default bodies of special member functions with = default;

https://reviewboard.mozilla.org/r/91680/#review92126

::: layout/base/nsRefreshDriver.cpp:520
(Diff revision 1)
>        if (!XRE_IsParentProcess()) {
>          mLastChildTick = TimeStamp::Now();
>        }
>      }
>    private:
> -    virtual ~RefreshDriverVsyncObserver() {}
> +    virtual ~RefreshDriverVsyncObserver() = default;

Is there a good reason for us to make this change, for destructors?

IMHO the current "{}" is just as clear as "= default", and it's shorter, too. :)  So, I don't see this class of changes (for empty destructors) as a clear win...

::: layout/style/nsCSSRuleProcessor.cpp:773
(Diff revision 1)
>    SelectorPair(const SelectorPair& aOther)
> -    : mSelector(aOther.mSelector)
> +    = default;
> -    , mRightmostSelector(aOther.mRightmostSelector) {}

Two nits:
 (1) Is there a point to leaving this copy-constructor declaration here at all, since we're using the default impl with default (public) visibility? I don't think there's a reason we need it, so we should probably just remove it.

 (2) If we were to keep the declaration, we'd probably want to bump "= default" up to the previous line. It feels a bit awkward on its own line here.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8808990 [details]
Bug 1316290 - Use C++11's override and remove already-implied virtual where applicable, in /layout.

https://reviewboard.mozilla.org/r/91682/#review92130

Patch looks good -- just some minor commit message tweaks:
> Bug 1316290 - Use C++11's override and remove virtual where applicable.

(1) add "already-implied" before "virtual", to make it clearer that this patch isn't actually changing behavior. (i.e. its removals are not devirtualizing any virtual functions)
(2) Add ", in /layout" to the end of the commit message (or something like that), to make the limited scope of this patch a bit clearer.

r=me, with that.

Comment 8

[Andi [:andi]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Comment on attachment 8808989 [details]
> Bug 1316290 - Replace default bodies of special member functions with =
> default;
> 
> https://reviewboard.mozilla.org/r/91680/#review92126
> 
> ::: layout/base/nsRefreshDriver.cpp:520
> (Diff revision 1)
> >        if (!XRE_IsParentProcess()) {
> >          mLastChildTick = TimeStamp::Now();
> >        }
> >      }
> >    private:
> > -    virtual ~RefreshDriverVsyncObserver() {}
> > +    virtual ~RefreshDriverVsyncObserver() = default;
> 
> Is there a good reason for us to make this change, for destructors?
> 
> IMHO the current "{}" is just as clear as "= default", and it's shorter,
> too. :)  So, I don't see this class of changes (for empty destructors) as a
> clear win...
> 
> ::: layout/style/nsCSSRuleProcessor.cpp:773
> (Diff revision 1)
> >    SelectorPair(const SelectorPair& aOther)
> > -    : mSelector(aOther.mSelector)
> > +    = default;
> > -    , mRightmostSelector(aOther.mRightmostSelector) {}
> 
> Two nits:
>  (1) Is there a point to leaving this copy-constructor declaration here at
> all, since we're using the default impl with default (public) visibility? I
> don't think there's a reason we need it, so we should probably just remove
> it.
> 
>  (2) If we were to keep the declaration, we'd probably want to bump "=
> default" up to the previous line. It feels a bit awkward on its own line
> here.

I see your point and if you consider we can have it dropped.

Comment 9

[Andi [:andi]]

Comment on attachment 8808988 [details]
Bug 1316290 - Use 'auto' type specifier for some variable decls that are assigned to "new", in /layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91678/diff/1-2/

Comment 10

[Andi [:andi]]

Comment on attachment 8808990 [details]
Bug 1316290 - Use C++11's override and remove already-implied virtual where applicable, in /layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91682/diff/1-2/

Comment 11

[Andi [:andi]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Comment on attachment 8808989 [details]
> Bug 1316290 - Replace default bodies of special member functions with =
> default;
> 
> https://reviewboard.mozilla.org/r/91680/#review92126
> 
> ::: layout/base/nsRefreshDriver.cpp:520
> (Diff revision 1)
> >        if (!XRE_IsParentProcess()) {
> >          mLastChildTick = TimeStamp::Now();
> >        }
> >      }
> >    private:
> > -    virtual ~RefreshDriverVsyncObserver() {}
> > +    virtual ~RefreshDriverVsyncObserver() = default;
> 
> Is there a good reason for us to make this change, for destructors?
> 
> IMHO the current "{}" is just as clear as "= default", and it's shorter,
> too. :)  So, I don't see this class of changes (for empty destructors) as a
> clear win...
> 
> ::: layout/style/nsCSSRuleProcessor.cpp:773
> (Diff revision 1)
> >    SelectorPair(const SelectorPair& aOther)
> > -    : mSelector(aOther.mSelector)
> > +    = default;
> > -    , mRightmostSelector(aOther.mRightmostSelector) {}
> 
> Two nits:
>  (1) Is there a point to leaving this copy-constructor declaration here at
> all, since we're using the default impl with default (public) visibility? I
> don't think there's a reason we need it, so we should probably just remove
> it.
> 
>  (2) If we were to keep the declaration, we'd probably want to bump "=
> default" up to the previous line. It feels a bit awkward on its own line
> here.
I know that the following will contradict my last post but still i want to keep =default where it's possible. If you check this link for clang-tidy, you will see by having =default some opportunities for optimisation may arise. http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default.html
This could impact our MAC OS builds that are done using clang/llvm.

Comment 12

[Andi [:andi]]

Attachment: Bug 1316290 - Replace default bodies of special member functions with = default, in layout.

Review commit: https://reviewboard.mozilla.org/r/92266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92266/

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8808988 [details]
Bug 1316290 - Use 'auto' type specifier for some variable decls that are assigned to "new", in /layout.

https://reviewboard.mozilla.org/r/91678/#review92378

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8809739 [details]
Bug 1316290 - Replace default bodies of special member functions with = default, in layout.

https://reviewboard.mozilla.org/r/92266/#review92380

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa3568c773f5
Use 'auto' type specifier for some variable decls that are assigned to "new", in /layout. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/b1e344ef212d
Use C++11's override and remove already-implied virtual where applicable, in /layout. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/19f65d152d88
Replace default bodies of special member functions with = default, in layout. r=dholbert

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa3568c773f5
https://hg.mozilla.org/mozilla-central/rev/b1e344ef212d
https://hg.mozilla.org/mozilla-central/rev/19f65d152d88