Convert some code in widget/ to C++11

RESOLVED FIXED in Firefox 53

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: sylvestre, Assigned: sylvestre)

Tracking

Trunk
mozilla53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(4 attachments)

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 #8811258 - Flags: review?(vladimir)
Attachment #8811261 - Flags: review?(vladimir)
Attachment #8811260 - Flags: review?(vladimir)
Attachment #8811259 - Flags: review?(vladimir)
Summary: Run clang-tidy on widget/ module → Convert some code in widget/ to C++11
Attachment #8811258 - Flags: review?(nical.bugzilla)
Attachment #8811261 - Flags: review?(vladimir) → review?(nical.bugzilla)
Attachment #8811260 - Flags: review?(vladimir)
Attachment #8811259 - Flags: review?(vladimir)
Attachment #8811260 - Flags: review?(tnikkel)
Attachment #8811259 - Flags: review?(tnikkel)
Assignee: nobody → sledru
Comment on attachment 8811259 [details]
Bug 1317973 - Use auto type specifier for variable declarations to improve code readability and maintainability

https://reviewboard.mozilla.org/r/93440/#review96684

Is this the style that we are going with now? I slightly prefer the existing code because the type is explicit and I don't have to guess the type when reading the code. But if there is consensus I won't go against it.
Comment on attachment 8811260 [details]
Bug 1317973 - Replace default bodies of special member functions with = default;

https://reviewboard.mozilla.org/r/93442/#review96686
Attachment #8811260 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tnikkel) from comment #16)
> Comment on attachment 8811259 [details]
> Is this the style that we are going with now? I slightly prefer the existing
> code because the type is explicit and I don't have to guess the type when
> reading the code. But if there is consensus I won't go against it.

We used the same approach as in LLVM: http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
tldr: when the type is explicitly set on the right part of the expression, we use auto. If it requires more thinking, we don't.
And yeah, we did that in various other places but we will start a thread on dev-platform about that to integrate this into the coding style
Comment on attachment 8811259 [details]
Bug 1317973 - Use auto type specifier for variable declarations to improve code readability and maintainability

https://reviewboard.mozilla.org/r/93440/#review96898

Alright if this is or will be the style.
Attachment #8811259 - Flags: review?(tnikkel) → review+
Comment on attachment 8811258 [details]
Bug 1317973 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11.

https://reviewboard.mozilla.org/r/93438/#review97082

::: widget/gtk/nsWindow.cpp:500
(Diff revision 3)
>  }
>  
>  /* static */ void
>  nsWindow::ReleaseGlobals()
>  {
> -  for (uint32_t i = 0; i < ArrayLength(gCursorCache); ++i) {
> +  for (auto & i : gCursorCache) {

There is a strong cognitive bias towards reading variables named "i" as integer indices. I would prefer loop variables like this one to be named "item" or something like that.
Attachment #8811258 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8811261 [details]
Bug 1317973 - Replace integer literals which are cast to bool.

https://reviewboard.mozilla.org/r/93444/#review97084
Attachment #8811261 - Flags: review?(nical.bugzilla) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b1670f7a0b4
Converts for(...; ...; ...) loops to use the new range-based loops in C++11. r=nical
https://hg.mozilla.org/integration/autoland/rev/d2bfabe4d015
Replace integer literals which are cast to bool. r=nical
https://hg.mozilla.org/integration/autoland/rev/8023e756661a
Replace default bodies of special member functions with = default; r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/9c229cf88ec2
Use auto type specifier for variable declarations to improve code readability and maintainability r=tnikkel
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.