Closed
Bug 1318004
Opened 4 years ago
Closed 4 years ago
Convert some code in toolkit/ to C++11
Categories
(Firefox Build System :: Source Code Analysis, defect)
Firefox Build System
Source Code Analysis
Tracking
(firefox53 fixed)
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: Sylvestre, Assigned: Sylvestre)
References
Details
Attachments
(4 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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•4 years ago
|
||
mozreview-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/#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 18•4 years ago
|
||
mozreview-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 19•4 years ago
|
||
mozreview-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 20•4 years ago
|
||
mozreview-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 21•4 years ago
|
||
mozreview-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+
Comment 22•4 years ago
|
||
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)
Comment 23•4 years ago
|
||
(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 | ||
Updated•4 years ago
|
Assignee: nobody → sledru
Flags: needinfo?(sledru)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•4 years ago
|
||
(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 30•4 years ago
|
||
mozreview-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/#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 31•4 years ago
|
||
mozreview-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+
Assignee | ||
Comment 32•4 years ago
|
||
> 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Attachment #8811314 -
Attachment is obsolete: true
Assignee | ||
Comment 37•4 years ago
|
||
mozreview-review-reply |
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 :/
Comment 38•4 years ago
|
||
(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...
Comment 39•4 years ago
|
||
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
I had to back these out for xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6953551&repo=autoland https://hg.mozilla.org/integration/autoland/rev/6a3911c19f7fd0ea8b679b7164740b19ee1dabbf
Flags: needinfo?(sledru)
Comment 41•4 years ago
|
||
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
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
Comment 43•4 years ago
|
||
(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.
Comment 44•4 years ago
|
||
(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. :-)
Comment hidden (mozreview-request) |
Comment 46•4 years ago
|
||
(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)
Comment 47•4 years ago
|
||
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
Comment 48•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d430f267791e https://hg.mozilla.org/mozilla-central/rev/f59ea728b758 https://hg.mozilla.org/mozilla-central/rev/bbb2524a8b4f https://hg.mozilla.org/mozilla-central/rev/3d3b3ea647d8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•3 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•