Closed Bug 1302401 Opened 8 years ago Closed 8 years ago

Try to run clang-tidy modernize on the js/ directory

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Attachments

(4 files, 14 obsolete files)

Bug 1302401 - Finds integer literals which are cast to bool.
58 bytes, text/x-review-board-request
Waldo
: review+
Details
Bug 1302401 - This check is responsible for using the auto type specifier for variable declarations to improve code readability and maintainability.
58 bytes, text/x-review-board-request
Waldo
: review+
Details
Bug 1302401 - This check converts for(...; ...; ...) loops to use the new range-based loops in C++11.
58 bytes, text/x-review-board-request
Waldo
: review+
Details
run-clang-tidy.sh
3.24 KB, application/x-shellscript
Details
Attached file run-clang-tidy.sh (obsolete) —
Similar to bug 1274087 with clang-format, here an experiment of clang-tidy. clang-tidy can automatically migrate the code to C++11 and do some various improvements in the code. I tried to run that on js/ as the code is pretty well localized. Note that I only run some checkers, not all of them. To run it: Build firefox with the option: ac_add_options --enable-build-backend=CompileDB it will generate compile_commands.json I wrote an ugly script to run it and commit it on a local git repo with these packages: http://apt.llvm.org/ apt-get install clang-tidy-4.0
Attachment #8790673 - Attachment is obsolete: true
Attachment #8790675 - Attachment is obsolete: true
Attachment #8790676 - Attachment is obsolete: true
Attachment #8790677 - Attachment is obsolete: true
Attachment #8790678 - Attachment is obsolete: true
Attachment #8790679 - Attachment is obsolete: true
Attachment #8790680 - Attachment is obsolete: true
Attachment #8790722 - Attachment is obsolete: true
Attachment #8791215 - Attachment is obsolete: true
Attachment #8791216 - Attachment is obsolete: true
Attachment #8791493 - Attachment is obsolete: true
These 4 patches didn't cause any regression on the code. However, * use nullptr when possible Fails on Windows with: 08:38:27 INFO - c:\builds\moz2_slave\try-w64-d-00000000000000000000\build\src\js\src\threading/Thread.h(237): error C2440: 'return': cannot convert from 'nullptr' to 'unsigned int' * This check replaces default bodies of special member functions with = default;" Failed on Windows: 07:12:41 INFO - c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/js/src/jit/x64/Trampoline-x64.cpp(727): error C2220: warning treated as error - no 'object' file generated 07:12:41 INFO - Warning: C4101 in c:\builds\moz2_slave\try-w64-d-00000000000000000000\build\src\js\src\jit\x64\Trampoline-x64.cpp: 'from': unreferenced local variable 07:12:41 INFO - c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/js/src/jit/x64/Trampoline-x64.cpp(727): warning C4101: 'from': unreferenced local variable 07:12:41 INFO - c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/config/rules.mk:950: recipe for target 'Unified_cpp_js_src23.obj' failed 07:12:41 INFO - mozmake.EXE[5]: *** [Unified_cpp_js_src23.obj] Error 2 * With move semantics added to the language and the standard library updated with move constructors added for many types it is now interesting to take an argument directly by value, instead of by const-reference, and then copy. Failed on Android 4.0 call to non-constexpr function 'typename std::__ndk1::remove_reference<_Tp>::type&& std::__ndk1::move(_Tp&&) [with _Tp = js::jit::RegisterSet&; typename std::__ndk1::remove_reference<_Tp>::type = js::jit::RegisterSet]' I will see for the remaining
Assignee: nobody → sledru
status-firefox51: affected → ---
status-firefox52: --- → affected
Attachment #8793889 - Attachment is obsolete: true
Comment on attachment 8790674 [details] Bug 1302401 - Finds integer literals which are cast to bool. https://reviewboard.mozilla.org/r/78374/#review79848
Attachment #8790674 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8793735 [details] Bug 1302401 - This check converts for(...; ...; ...) loops to use the new range-based loops in C++11. https://reviewboard.mozilla.org/r/80412/#review79852 ::: js/src/perf/pm_linux.cpp:125 (Diff revision 1) > { > // Close all active counter descriptors. Take care to do the group > // leader last (this may not be necessary, but it's unclear what > // happens if you close the group leader out from under a group). > - for (int i = 0; i < PerfMeasurement::NUM_MEASURABLE_EVENTS; i++) { > - int fd = this->*(kSlots[i].fd); > + for (const auto & kSlot : kSlots) { > + int fd = this->*(kSlot.fd); Using ranged syntax for these isn't bad, but two changes are definitely required to this automatically-generated patch. Each variable should be named |slot|, not |kSlot| -- "k" prefix indicates constant, but that's not really true here. The ampersand should be immediately adjacent to |auto|, i.e. for (const auto& slot : kSlots) {
Attachment #8793735 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8793209 [details] Bug 1302401 - This check selectively replaces string literals containing escaped characters with raw string literals. https://reviewboard.mozilla.org/r/80020/#review79860 I think SpiderMonkey hackers generally aren't sufficiently familiar with C++11's raw string syntax for us to start doing this just now. Beyond that, having discussed the syntax in #jsapi just now, I think the consensus is the syntax is pretty grody. Those concerns may perhaps diminish with time. But. If those concerns eventually dissipate, I think generally we don't find some of these syntaxes very readable *for some of these uses*, *because* some of the parentheses/quotes that are supposed to be in the string, blend in with the raw-string syntax super-horribly. I don't think anyone likes R"(")" compared to "\\"", or R"("))" compared to "\")", for example. So, no -- ixnay on this particular angechay.
Attachment #8793209 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8793254 [details] Bug 1302401 - This check is responsible for using the auto type specifier for variable declarations to improve code readability and maintainability. https://reviewboard.mozilla.org/r/80052/#review79866 ::: js/src/jit/LIR.h:1817 (Diff revision 1) > } > uint32_t numBlockIds() const { > return mir_.numBlockIds(); > } > MOZ_MUST_USE bool initBlock(MBasicBlock* mir) { > - LBlock* lir = new (&blocks_[mir->id()]) LBlock(mir); > + auto* lir = new (&blocks_[mir->id()]) LBlock(mir); The other cases aren't, I think, too objectionable. The new-operator is only provided a single argument -- a single name -- so it's not too difficult to scan and find the type of the memory being filled. This instance, however, reads a bit messy as to the argument passed in -- multiple names, multiple delimiters (brackets, braces) piling up at the end of the new-arguments. Too much for easy skimmability, IMO. But that's solvable -- just put the new-operator argument in a fresh variable: auto* block = &blocks_[mir->id()]; new (block) LBlock(mir); return lir->init(mir_.alloc()); r=me with that change.
Attachment #8793254 - Flags: review?(jwalden+bmo) → review+
Attachment #8793209 - Attachment is obsolete: true
Attachment #8793209 - Flags: review?(jwalden+bmo)
Thanks. I removed the raw string patch + fixed the auto declaration + renamed kSlot to slot I will wait for your review as you r- one of them.
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8793735 [details] Bug 1302401 - This check converts for(...; ...; ...) loops to use the new range-based loops in C++11. https://reviewboard.mozilla.org/r/80412/#review80390
Attachment #8793735 - Flags: review?(jwalden+bmo) → review+
Flags: needinfo?(jwalden+bmo)
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/266b54a73434 Finds integer literals which are cast to bool. r=Waldo https://hg.mozilla.org/integration/autoland/rev/7d8b27826d09 This check is responsible for using the auto type specifier for variable declarations to improve code readability and maintainability. r=Waldo https://hg.mozilla.org/integration/autoland/rev/74d398e52502 This check converts for(...; ...; ...) loops to use the new range-based loops in C++11. r=Waldo
Phil, I am not sure why you backout that. My try job worked without any issue: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4fafab6b47b
Flags: needinfo?(philringnalda)
Your try job worked without any WinXP tests (because we solve testing capacity issues by just shutting entire platforms off on try by default, requiring you to specifically request them), and without any Marionette tests (because... why would you expect to need to run them?). But I backed you out because https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=5f49c895693892920b3710b6e713f2972cc606a9&filter-searchStr=19a5c36317f26393e00f6ae0960d0ed7fb421e85&tochange=ccc7ffa3eee6b51dec4e404e92ec97caf5045fc6 said to: WinXP Mn was green, then there was a Fennec-only patch, then your three patches, and starting on your push and continuing until I backed it out, every single WinXP Mn failed in the same test. Of course, strange things happen, and maybe my backout won't fix that failure, but I did retrigger the test on the last push which was successful, and got another success, so if backing you out doesn't fix it I'm going to have to assume that a patch only touching mobile/android/base/java/org/mozilla/gecko/delegates/OfflineTabStatusDelegate.java caused a failure on Windows, which would be about a billion times more surprising than one of your patches causing it.
Flags: needinfo?(philringnalda)
Fun, it's still busted on my backout, so either a patch to a file not built on Windows did it, or eleven times in a row we got broken slaves while getting an unbroken one on the one run I retriggered on the last green, or... I'll have to come up with an even wilder theory.
I'm baffled. 25 of 25 green on the push below you; 25 of 25 failed on your push; 9 of 10 failed on my backout. I'd go for the last desperate guess, backing out required a clobber (and in fact I did clobber, just out of desperation), but having 1 of 10 green on the backout makes that seem less likely.
Ah ah, my guess is that it is something unrelated (infra or a system issue?)
Pushed by sledru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7c71ddd0c30f Finds integer literals which are cast to bool. r=Waldo https://hg.mozilla.org/integration/autoland/rev/3d533ce74532 This check is responsible for using the auto type specifier for variable declarations to improve code readability and maintainability. r=Waldo https://hg.mozilla.org/integration/autoland/rev/9c1d4bb7c4ee This check converts for(...; ...; ...) loops to use the new range-based loops in C++11. r=Waldo
A treeherder race condition that causes it to not show a push, so there was a totally obvious cause for it, pushed before your push and after the one shown as being the one before you, just invisible.
no worries. did you report a bug to avoid that in the future?
(In reply to Sylvestre Ledru [:sylvestre] from comment #51) > no worries. > did you report a bug to avoid that in the future? For anyone following along, that is https://bugzilla.mozilla.org/show_bug.cgi?id=1286578
Attached file run-clang-tidy.sh —
here is an updated version of the script I used
Attachment #8790672 - Attachment is obsolete: true
Blocks: c++11-migration
No longer blocks: clang-based-analysis
Comment on attachment 8790674 [details] Bug 1302401 - Finds integer literals which are cast to bool. https://reviewboard.mozilla.org/r/78374/#review97512
Product: Core → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: