Closed Bug 1316290 Opened 3 years ago Closed 3 years ago

Run clang-tidy on layout/ module

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: andi, Assigned: andi)

References

Details

Attachments

(3 files, 1 obsolete file)

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
Attachment #8808988 - Flags: review?(dholbert)
Attachment #8808989 - Flags: review?(dholbert)
Attachment #8808990 - Flags: review?(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
Attachment #8808988 - Flags: review?(dholbert) → review-
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 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.
Attachment #8808990 - Flags: review?(dholbert) → review+
(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.
Attachment #8808989 - Attachment is obsolete: true
Attachment #8808989 - Flags: review?(dholbert)
(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 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
Attachment #8808988 - Flags: review?(dholbert) → review+
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
Attachment #8809739 - Flags: review?(dholbert) → review+
Pushed by bpostelnicu@mozilla.com:
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
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.