Closed Bug 1318004 Opened 5 years ago Closed 5 years ago

Convert some code in toolkit/ to C++11

Categories

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

Product:
Firefox Build System
Component:
Source Code Analysis
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1318004 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11.
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1318004 - Replace default bodies of special member functions with = default;
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1318004 - Replace string literals containing escaped characters with raw string literals.
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1318004 - Use C++11's override and remove virtual where applicable.
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details 
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 #8811313 - Flags: review?(ehsan)
Attachment #8811314 - Flags: review?(ehsan)
Attachment #8811315 - Flags: review?(ehsan)
Attachment #8811316 - Flags: review?(ehsan)
Attachment #8811317 - Flags: review?(ehsan)
Comment on attachment 8811313 [details]
Bug 1318004 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11.

https://reviewboard.mozilla.org/r/93466/#review93566

r- because it seems that you didn't really look over the changes made by the tool here.  :-)

::: toolkit/components/telemetry/Telemetry.cpp:1730
(Diff revision 2)
>      }
>    }
>  
>    // Add saved threads next
>    MutexAutoLock autoLock(mThreadHangStatsMutex);
> -  for (size_t i = 0; i < mThreadHangStats.length(); i++) {
> +  for (auto & mThreadHangStat : mThreadHangStats) {

This isn't a good name, since the m prefix is reserved for data members.  Please call it something else instead (such as |stat|).

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2164
(Diff revision 2)
>  
>  void
>  TelemetryHistogram::InitHistogramRecordingEnabled()
>  {
>    StaticMutexAutoLock locker(gTelemetryHistogramMutex);
> -  const size_t length = mozilla::ArrayLength(kRecordingInitiallyDisabledIDs);
> +  for (auto kRecordingInitiallyDisabledID : kRecordingInitiallyDisabledIDs) {

Another bad name!  The prefix k is reserved for constants.

::: toolkit/components/terminator/nsTerminator.cpp:345
(Diff revision 2)
>    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>    if (!os) {
>      return NS_ERROR_UNEXPECTED;
>    }
>  
> -  for (size_t i = 0; i < ArrayLength(sShutdownSteps); ++i) {
> +  for (auto & sShutdownStep : sShutdownSteps) {

Another one.

::: toolkit/components/terminator/nsTerminator.cpp:510
(Diff revision 2)
>  
>    // Build JSON.
>    UniquePtr<nsCString> telemetryData(new nsCString());
>    telemetryData->AppendLiteral("{");
>    size_t fields = 0;
> -  for (size_t i = 0; i < ArrayLength(sShutdownSteps); ++i) {
> +  for (auto & sShutdownStep : sShutdownSteps) {

And more.

::: toolkit/system/gnome/nsGConfService.cpp:85
(Diff revision 2)
>      gconfLib = PR_LoadLibrary("libgconf-2.so.4");
>      if (!gconfLib)
>        return NS_ERROR_FAILURE;
>    }
>  
> -  for (uint32_t i = 0; i < ArrayLength(kGConfSymbols); i++) {
> +  for (auto kGConfSymbol : kGConfSymbols) {

And more.

::: toolkit/system/gnome/nsGSettingsService.cpp:311
(Diff revision 2)
>      gioLib = PR_LoadLibrary("libgio-2.0.so.0");
>      if (!gioLib)
>        return NS_ERROR_FAILURE;
>    }
>  
> -  for (uint32_t i = 0; i < ArrayLength(kGSettingsSymbols); i++) {
> +  for (auto kGSettingsSymbol : kGSettingsSymbols) {

Yet another.

::: toolkit/system/gnome/nsPackageKitService.cpp:96
(Diff revision 2)
>      gioLib = PR_LoadLibrary("libgio-2.0.so.0");
>      if (!gioLib)
>        return NS_ERROR_FAILURE;
>    }
>  
> -  for (uint32_t i = 0; i < ArrayLength(kGDBusSymbols); i++) {
> +  for (auto kGDBusSymbol : kGDBusSymbols) {

Ditto.

::: toolkit/xre/nsAppRunner.cpp:2751
(Diff revision 2)
>    {"XUL_APP_FILE", nullptr}
>  };
>  
>  static void SaveStateForAppInitiatedRestart()
>  {
> -  for (size_t i = 0; i < ArrayLength(gSavedVars); ++i) {
> +  for (auto & gSavedVar : gSavedVars) {

This one too.

::: toolkit/xre/nsAppRunner.cpp:2760
(Diff revision 2)
>    }
>  }
>  
>  static void RestoreStateForAppInitiatedRestart()
>  {
> -  for (size_t i = 0; i < ArrayLength(gSavedVars); ++i) {
> +  for (auto & gSavedVar : gSavedVars) {

And this one.
Attachment #8811313 - Flags: review?(ehsan) → review-
Comment on attachment 8811314 [details]
Bug 1318004 - Use auto type specifier for variable declarations to improve code readability and maintainability

https://reviewboard.mozilla.org/r/93468/#review93568

::: toolkit/components/telemetry/TelemetryHistogram.cpp:2372
(Diff revision 2)
>      const uint32_t type = gHistograms[i].histogramType;
>      if (type == nsITelemetry::HISTOGRAM_FLAG ||
>          type == nsITelemetry::HISTOGRAM_COUNT) {
>        Histogram *h;
>        mozilla::DebugOnly<nsresult> rv;
> -      mozilla::Telemetry::ID id = mozilla::Telemetry::ID(i);
> +      auto id = mozilla::Telemetry::ID(i);

This clang-tidy tool often times generates code that's less readable than it was before, so we definitely don't want to take its output en masse like this.

For example, this code should be rewritten as follows instead:

  mozilla::Telemetry::ID id(i);

Another example immediately after should be written like this:

  auto scalar = static_cast<ScalarBase*>(iter.Data());

And so on.  Please go through the patch manually and make sure that the output is actually good C++.  In all honesty, I suggest just not running this tool, since it takes quite a bit of manual work to make its output resemble well written C++.
Attachment #8811314 - Flags: review?(ehsan) → review-
Comment on attachment 8811315 [details]
Bug 1318004 - Replace default bodies of special member functions with = default;

https://reviewboard.mozilla.org/r/93470/#review93570

::: toolkit/components/telemetry/TelemetryScalar.cpp:158
(Diff revision 4)
>   * purposes.
>   */
>  class ScalarBase
>  {
>  public:
> -  virtual ~ScalarBase() {};
> +  virtual ~ScalarBase() = default;;

Bad output (note the double semi-colon) here and in a bunch of places.  It seems like you should fix clang-tidy here.  r+ with these fixed either manually or through fixing the tool.

::: toolkit/components/terminator/nsTerminator.cpp:179
(Diff revision 4)
>  {
>  public:
> -  constexpr PR_CloseDelete() {}
> +  constexpr PR_CloseDelete() = default;
>  
>    PR_CloseDelete(const PR_CloseDelete& aOther)
> -  {}
> +  = default;

These must go on the same line (here and in a bunch of other places too.)  Looks like another thing to fix in clang-tidy.
Attachment #8811315 - Flags: review?(ehsan) → review+
Comment on attachment 8811316 [details]
Bug 1318004 - Replace string literals containing escaped characters with raw string literals.

https://reviewboard.mozilla.org/r/93472/#review93572
Attachment #8811316 - Flags: review?(ehsan) → review+
Comment on attachment 8811317 [details]
Bug 1318004 - Use C++11's override and remove virtual where applicable.

https://reviewboard.mozilla.org/r/93474/#review93596

Have you seen bug 1158776?  It would be great if you can finish that.  :-)
Attachment #8811317 - Flags: review?(ehsan) → review+
Can you also add a .clang-tidy file corresponding to the checks run here similar to the one added in bug 1158656?
Flags: needinfo?(sledru)
(In reply to :Ehsan Akhgari from comment #18)
> Comment on attachment 8811314 [details]
> Bug 1318004 - Use auto type specifier for variable declarations to improve
> code readability and maintainability
> 
> https://reviewboard.mozilla.org/r/93468/#review93568
> 
> ::: toolkit/components/telemetry/TelemetryHistogram.cpp:2372
> (Diff revision 2)
> >      const uint32_t type = gHistograms[i].histogramType;
> >      if (type == nsITelemetry::HISTOGRAM_FLAG ||
> >          type == nsITelemetry::HISTOGRAM_COUNT) {
> >        Histogram *h;
> >        mozilla::DebugOnly<nsresult> rv;
> > -      mozilla::Telemetry::ID id = mozilla::Telemetry::ID(i);
> > +      auto id = mozilla::Telemetry::ID(i);
> 
> This clang-tidy tool often times generates code that's less readable than it
> was before, so we definitely don't want to take its output en masse like
> this.
> 
> For example, this code should be rewritten as follows instead:
> 
>   mozilla::Telemetry::ID id(i);
> 
> Another example immediately after should be written like this:
> 
>   auto scalar = static_cast<ScalarBase*>(iter.Data());
> 
> And so on.  Please go through the patch manually and make sure that the
> output is actually good C++.  In all honesty, I suggest just not running
> this tool, since it takes quite a bit of manual work to make its output
> resemble well written C++.

I see your point here but i would prefer to have auto followed by *, since we can have situations like this:

>> auto var1 = SomeNameSpace::SomeClass::GetSomething()

Where get something return type can be let's say int* but looking at a glance at the code we don't realise the type of var1, and by having the type followed by the pointer qualifier i think it's more visible and easy to read.
Assignee: nobody → sledru
Flags: needinfo?(sledru)
(In reply to Andi-Bogdan Postelnicu from comment #23)
> (In reply to :Ehsan Akhgari from comment #18)
> > Comment on attachment 8811314 [details]
> > Bug 1318004 - Use auto type specifier for variable declarations to improve
> > code readability and maintainability
> > 
> > https://reviewboard.mozilla.org/r/93468/#review93568
> > 
> > ::: toolkit/components/telemetry/TelemetryHistogram.cpp:2372
> > (Diff revision 2)
> > >      const uint32_t type = gHistograms[i].histogramType;
> > >      if (type == nsITelemetry::HISTOGRAM_FLAG ||
> > >          type == nsITelemetry::HISTOGRAM_COUNT) {
> > >        Histogram *h;
> > >        mozilla::DebugOnly<nsresult> rv;
> > > -      mozilla::Telemetry::ID id = mozilla::Telemetry::ID(i);
> > > +      auto id = mozilla::Telemetry::ID(i);
> > 
> > This clang-tidy tool often times generates code that's less readable than it
> > was before, so we definitely don't want to take its output en masse like
> > this.
> > 
> > For example, this code should be rewritten as follows instead:
> > 
> >   mozilla::Telemetry::ID id(i);
> > 
> > Another example immediately after should be written like this:
> > 
> >   auto scalar = static_cast<ScalarBase*>(iter.Data());
> > 
> > And so on.  Please go through the patch manually and make sure that the
> > output is actually good C++.  In all honesty, I suggest just not running
> > this tool, since it takes quite a bit of manual work to make its output
> > resemble well written C++.
> 
> I see your point here but i would prefer to have auto followed by *, since
> we can have situations like this:
> 
> >> auto var1 = SomeNameSpace::SomeClass::GetSomething()
> 
> Where get something return type can be let's say int* but looking at a
> glance at the code we don't realise the type of var1, and by having the type
> followed by the pointer qualifier i think it's more visible and easy to read.

This is why I think this clang-tidy tool is far from generating something useful.  I agree that in your above example, you can auto* and not auto.  But with this example:

  auto* foo = static_cast<Foo*>(...);

you don't want to say that foo is a pointer, since the static_cast is doing that...

Also, in some cases switching to auto can make the resulting code less readable since sometimes seeing the type delivers useful information to the reader of the code.  For example if you have a Foo and an UnsafeFoo type, with UnsafeFoo having restrictions on how it should be used, you don't want to delete the type information from such local vars in some cases.
Comment on attachment 8811314 [details]
Bug 1318004 - Use auto type specifier for variable declarations to improve code readability and maintainability

https://reviewboard.mozilla.org/r/93468/#review94612

::: toolkit/components/commandlines/nsCommandLine.cpp:625
(Diff revision 3)
>    rv = aHandler->GetHelpInfo(text);
>    if (NS_SUCCEEDED(rv)) {
>      NS_ASSERTION(text.Length() == 0 || text.Last() == '\n',
>                   "Help text from command line handlers should end in a newline.");
>  
> -    nsACString* totalText = reinterpret_cast<nsACString*>(aClosure);
> +    auto* totalText = reinterpret_cast<nsACString*>(aClosure);

auto, not auto*, as I mentioned in comment 18.

::: toolkit/components/ctypes/ctypes.cpp:37
(Diff revision 3)
>    if (NS_FAILED(rv)) {
>      JS_ReportErrorASCII(cx, "could not convert string to native charset");
>      return nullptr;
>    }
>  
> -  char* result = static_cast<char*>(JS_malloc(cx, native.Length() + 1));
> +  auto* result = static_cast<char*>(JS_malloc(cx, native.Length() + 1));

Same

::: toolkit/components/downloads/SQLFunctions.cpp:116
(Diff revision 3)
>  {
>    _guid.Truncate();
>  
>    // Request raw random bytes and base64url encode them.  For each set of three
>    // bytes, we get one character.
> -  const uint32_t kRequiredBytesLength =
> +  const auto kRequiredBytesLength =

Again, please see comment 18.

::: toolkit/components/osfile/NativeOSFileInternals.cpp:687
(Diff revision 3)
>        return NS_ERROR_FAILURE;
>      }
>  
>      uint64_t total_read = 0;
>      int32_t just_read = 0;
> -    char* dest_chars = reinterpret_cast<char*>(aBuffer.rwget().data);
> +    auto* dest_chars = reinterpret_cast<char*>(aBuffer.rwget().data);

OK, I'm not really sure if you even read comment 18 at this point.  :-)  Minusing without looking at the rest.

Again, my recommendation is to *not* run this tool.  I don't think it's worth any of our times iterating over this stuff.
Attachment #8811314 - Flags: review?(ehsan) → review-
Comment on attachment 8811313 [details]
Bug 1318004 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11.

https://reviewboard.mozilla.org/r/93466/#review94614
Attachment #8811313 - Flags: review?(ehsan) → review+
> OK, I'm not really sure if you even read comment 18 at this point.  :-)  Minusing without looking at the 
> rest.
I did, Andi answered for me (he has been leading the technical efforts)
Sorry if this wasn't clear. I am just going to drop this patch then.
Attachment #8811314 - Attachment is obsolete: true
Comment on attachment 8811313 [details]
Bug 1318004 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11.

https://reviewboard.mozilla.org/r/93466/#review93566

I am not familar with the naming convention, sorry :/
(In reply to Sylvestre Ledru [:sylvestre] from comment #32)
> > OK, I'm not really sure if you even read comment 18 at this point.  :-)  Minusing without looking at the 
> > rest.
> I did, Andi answered for me (he has been leading the technical efforts)
> Sorry if this wasn't clear. I am just going to drop this patch then.

Cool, no worries!

(In reply to Sylvestre Ledru [:sylvestre] from comment #37)
> Comment on attachment 8811313 [details]
> Bug 1318004 - Converts for(...; ...; ...) loops to use the new range-based
> loops in C++11.
> 
> https://reviewboard.mozilla.org/r/93466/#review93566
> 
> I am not familar with the naming convention, sorry :/

Hmm,, there were no naming convention issues in this patch...
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f2ebf176eac
Replace default bodies of special member functions with = default; r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/365b0b7f289a
Replace string literals containing escaped characters with raw string literals. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/9be5c856bc78
Use C++11's override and remove virtual where applicable. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/6995bd33872b
Converts for(...; ...; ...) loops to use the new range-based loops in C++11. r=Ehsan
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb5254baf1cb
Replace default bodies of special member functions with = default; r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/b56b37f0f4d7
Replace string literals containing escaped characters with raw string literals. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/a5d1676b6f88
Use C++11's override and remove virtual where applicable. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/a3fe53641526
Converts for(...; ...; ...) loops to use the new range-based loops in C++11. r=Ehsan
(In reply to :Ehsan Akhgari from comment #29)
> (In reply to Andi-Bogdan Postelnicu from comment #23)
> > (In reply to :Ehsan Akhgari from comment #18)
> > > Comment on attachment 8811314 [details]
> > > Bug 1318004 - Use auto type specifier for variable declarations to improve
> > > code readability and maintainability
> > > 
> > > https://reviewboard.mozilla.org/r/93468/#review93568
> > > 
> > > ::: toolkit/components/telemetry/TelemetryHistogram.cpp:2372
> > > (Diff revision 2)
> > > >      const uint32_t type = gHistograms[i].histogramType;
> > > >      if (type == nsITelemetry::HISTOGRAM_FLAG ||
> > > >          type == nsITelemetry::HISTOGRAM_COUNT) {
> > > >        Histogram *h;
> > > >        mozilla::DebugOnly<nsresult> rv;
> > > > -      mozilla::Telemetry::ID id = mozilla::Telemetry::ID(i);
> > > > +      auto id = mozilla::Telemetry::ID(i);
> > > 
> > > This clang-tidy tool often times generates code that's less readable than it
> > > was before, so we definitely don't want to take its output en masse like
> > > this.
> > > 
> > > For example, this code should be rewritten as follows instead:
> > > 
> > >   mozilla::Telemetry::ID id(i);
> > > 
> > > Another example immediately after should be written like this:
> > > 
> > >   auto scalar = static_cast<ScalarBase*>(iter.Data());
> > > 
> > > And so on.  Please go through the patch manually and make sure that the
> > > output is actually good C++.  In all honesty, I suggest just not running
> > > this tool, since it takes quite a bit of manual work to make its output
> > > resemble well written C++.
> > 
> > I see your point here but i would prefer to have auto followed by *, since
> > we can have situations like this:
> > 
> > >> auto var1 = SomeNameSpace::SomeClass::GetSomething()
> > 
> > Where get something return type can be let's say int* but looking at a
> > glance at the code we don't realise the type of var1, and by having the type
> > followed by the pointer qualifier i think it's more visible and easy to read.
> 
> This is why I think this clang-tidy tool is far from generating something
> useful.  I agree that in your above example, you can auto* and not auto. 
> But with this example:
> 
>   auto* foo = static_cast<Foo*>(...);
> 
> you don't want to say that foo is a pointer, since the static_cast is doing
> that...
> 
> Also, in some cases switching to auto can make the resulting code less
> readable since sometimes seeing the type delivers useful information to the
> reader of the code.  For example if you have a Foo and an UnsafeFoo type,
> with UnsafeFoo having restrictions on how it should be used, you don't want
> to delete the type information from such local vars in some cases.

I don't want to sound that I'm pushing this and i agree with you that in some cases clang-tidy can do more harm than good regarding code readability. But dispite this we already pushed several patches to m-c that respect this methodology like:

>>auto& a = something();

or 
>>auto* b = something();

https://bugzilla.mozilla.org/show_bug.cgi?id=1311669
https://bugzilla.mozilla.org/show_bug.cgi?id=1302401
https://bugzilla.mozilla.org/show_bug.cgi?id=1316290
https://bugzilla.mozilla.org/show_bug.cgi?id=1317241
https://bugzilla.mozilla.org/show_bug.cgi?id=1317637
https://bugzilla.mozilla.org/show_bug.cgi?id=1317954

I think it would be nice to have the same coding style across all our code.
(In reply to Andi-Bogdan Postelnicu from comment #43)
> (In reply to :Ehsan Akhgari from comment #29)
> > (In reply to Andi-Bogdan Postelnicu from comment #23)
> > > (In reply to :Ehsan Akhgari from comment #18)
> > > > Comment on attachment 8811314 [details]
> > > > Bug 1318004 - Use auto type specifier for variable declarations to improve
> > > > code readability and maintainability
> > > > 
> > > > https://reviewboard.mozilla.org/r/93468/#review93568
> > > > 
> > > > ::: toolkit/components/telemetry/TelemetryHistogram.cpp:2372
> > > > (Diff revision 2)
> > > > >      const uint32_t type = gHistograms[i].histogramType;
> > > > >      if (type == nsITelemetry::HISTOGRAM_FLAG ||
> > > > >          type == nsITelemetry::HISTOGRAM_COUNT) {
> > > > >        Histogram *h;
> > > > >        mozilla::DebugOnly<nsresult> rv;
> > > > > -      mozilla::Telemetry::ID id = mozilla::Telemetry::ID(i);
> > > > > +      auto id = mozilla::Telemetry::ID(i);
> > > > 
> > > > This clang-tidy tool often times generates code that's less readable than it
> > > > was before, so we definitely don't want to take its output en masse like
> > > > this.
> > > > 
> > > > For example, this code should be rewritten as follows instead:
> > > > 
> > > >   mozilla::Telemetry::ID id(i);
> > > > 
> > > > Another example immediately after should be written like this:
> > > > 
> > > >   auto scalar = static_cast<ScalarBase*>(iter.Data());
> > > > 
> > > > And so on.  Please go through the patch manually and make sure that the
> > > > output is actually good C++.  In all honesty, I suggest just not running
> > > > this tool, since it takes quite a bit of manual work to make its output
> > > > resemble well written C++.
> > > 
> > > I see your point here but i would prefer to have auto followed by *, since
> > > we can have situations like this:
> > > 
> > > >> auto var1 = SomeNameSpace::SomeClass::GetSomething()
> > > 
> > > Where get something return type can be let's say int* but looking at a
> > > glance at the code we don't realise the type of var1, and by having the type
> > > followed by the pointer qualifier i think it's more visible and easy to read.
> > 
> > This is why I think this clang-tidy tool is far from generating something
> > useful.  I agree that in your above example, you can auto* and not auto. 
> > But with this example:
> > 
> >   auto* foo = static_cast<Foo*>(...);
> > 
> > you don't want to say that foo is a pointer, since the static_cast is doing
> > that...
> > 
> > Also, in some cases switching to auto can make the resulting code less
> > readable since sometimes seeing the type delivers useful information to the
> > reader of the code.  For example if you have a Foo and an UnsafeFoo type,
> > with UnsafeFoo having restrictions on how it should be used, you don't want
> > to delete the type information from such local vars in some cases.
> 
> I don't want to sound that I'm pushing this and i agree with you that in
> some cases clang-tidy can do more harm than good regarding code readability.
> But dispite this we already pushed several patches to m-c that respect this
> methodology like:
> 
> >>auto& a = something();
> 
> or 
> >>auto* b = something();
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1311669
> https://bugzilla.mozilla.org/show_bug.cgi?id=1302401
> https://bugzilla.mozilla.org/show_bug.cgi?id=1316290
> https://bugzilla.mozilla.org/show_bug.cgi?id=1317241
> https://bugzilla.mozilla.org/show_bug.cgi?id=1317637
> https://bugzilla.mozilla.org/show_bug.cgi?id=1317954
> 
> I think it would be nice to have the same coding style across all our code.

Yes, I agree that it would be nice to have the same coding style.  I haven't seen any of the above bugs, and I haven't seen this being discussed anywhere, so you shouldn't automatically assume that I agree with what has already happened above.  ;-)

Historically every time we have discussed anything about our coding style we have managed to find vocal proponents in all sides of the conversation and the conversation typically ends up in people disagreeing and nothing moving forward.  We need to have someone who owns making a final decision to be able to have another one of these discussions productively.  That can in theory be solve when we officiate the C++/Rust Tools, Usage and Style module.  Then we also need to find a way to get people to agree with the final decision, which would be a nice exercise.  :-)
(In reply to Wes Kocher (:KWierso) from comment #42)
> Backed out for xpcshell failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=6981496&repo=autoland
> 
> https://hg.mozilla.org/integration/autoland/rev/
> c22454e450c6415280d30ceae8ed3d566ca7dd67

this has been fixed, so we can push this to m-i.
Flags: needinfo?(sledru)
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d430f267791e
Replace default bodies of special member functions with = default; r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/f59ea728b758
Replace string literals containing escaped characters with raw string literals. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/bbb2524a8b4f
Use C++11's override and remove virtual where applicable. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/3d3b3ea647d8
Converts for(...; ...; ...) loops to use the new range-based loops in C++11. r=Ehsan
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.