Page MenuHomePhabricator Dev
Differential D714

Bug 1387052 - Make Firefox code worse intentionally 1/2.
Needs ReviewPublic
Actions

Authored by janx on Oct 5 2018, 8:36 AM.
Tags
None
Referenced Files
F67106187: D714.1792475896.diff
Mon, Oct 19, 5:58 AM
Unknown Object (File)
Thu, Oct 8, 4:26 PM
Unknown Object (File)
Jul 24 2026, 9:17 AM
Unknown Object (File)
Jul 20 2026, 8:18 AM
Unknown Object (File)
Jul 17 2026, 9:37 AM
Unknown Object (File)
Jun 8 2026, 11:20 AM
Unknown Object (File)
Jun 7 2026, 5:14 AM
Unknown Object (File)
May 23 2026, 8:59 AM
View All 27 Files
Subscribers
None

Details

Reviewers
ReviewBot
Bugzilla Bug ID
1387052

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ReviewBot added inline comments.Oct 10 2018, 12:46 PM
browser/base/content/docs/tabbrowser/index.rst
7

Error: Exisits ==> exists [codespell]

devtools/client/definitions.js
7

Error: Infix operators must be spaced. [eslint: space-infix-ops]

devtools/client/menus.js
41

Error: Unexpected tab character. [eslint: no-tabs]

devtools/client/shared/scroll.js
7

Error: Parsing error: Unexpected character '#' [eslint]

js/ductwork/debugger/JSDebugger.cpp
50

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

else if (!JS_IsGlobalObject(obj)) { // after: readability-else-after-return
^~~~~~~~
js/src/jsapi.cpp
142

Error: Methods annotated with MOZ_NO_DANGLING_ON_TEMPORARIES cannot be && ref-qualified [clang-tidy: mozilla-dangling-on-temporary]

MOZ_NO_DANGLING_ON_TEMPORARIES int *get() && { return nullptr; } // after: mozilla-dangling-on-temporary
                                    ^
143

Error: Methods annotated with MOZ_NO_DANGLING_ON_TEMPORARIES must return a pointer [clang-tidy: mozilla-dangling-on-temporary]

MOZ_NO_DANGLING_ON_TEMPORARIES int test() { return 0; } // after: mozilla-dangling-on-temporary
                                   ^
233

Warning: Potentially unintended semicolon [clang-tidy: bugprone-suspicious-semicolon]

if (x < y); // after: bugprone-suspicious-semicolon; readability-braces-around-statements
          ^
233

Warning: Statement should be inside braces [clang-tidy: readability-braces-around-statements]

if (x < y); // after: bugprone-suspicious-semicolon; readability-braces-around-statements
          ^
           {
356

Warning: 'find' called with a string literal consisting of a single character; consider using the more effective overload accepting a character [clang-tidy: performance-faster-string-find]

str.find("A"); // after: performance-faster-string-find
         ^~~~
         'A'
365

Warning: The type of the loop variable 'p' is different from the one returned by the iterator and generates an implicit conversion; you can either change the type to the matching one ('const std::pair<const int, std::vector<std::cxx11::basic_string<char>, std::allocator<std::cxx11::basic_string<char> > > > &' but 'const auto&' is always a valid option) or remove the reference to make it explicit that you are creating a new value [clang-tidy: performance-implicit-conversion-in-loop]

for (const std::pair<int, std::vector<std::string>>& p : my_map) {} // after: performance-implicit-conversion-in-loop
     ^
408

Warning: Implicit conversion 'JSObject *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!obj) {
    ~^~~~
     == nullptr
521

Warning: Implicit conversion 'JSRuntime *' -> bool [clang-tidy: readability-implicit-bool-conversion]

while (parentRuntime && parentRuntime->parentRuntime) {
       ^~~~~~~~~~~~~
       ( != nullptr)
603

Warning: Implicit conversion 'js::jit::JitRuntime *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!rt->getJitRuntime(cx)) {
    ~^~                   ~
                           == nullptr
612

Warning: Implicit conversion 'JSRuntime *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!rt->parentRuntime && !rt->initMainAtomsTables(cx)) {
    ~^~
    (                  == nullptr)
740

Warning: Implicit conversion 'JSObject *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (targetOrNull) {
    ^           ~
                 != nullptr
1077

Warning: Implicit conversion 'const JSStdName *' -> bool [clang-tidy: readability-implicit-bool-conversion]

JSProtoKey key = stdnm ? stdnm->key : JSProto_Null;
                 ^
                       != nullptr
1080

Warning: Implicit conversion 'const js::Class *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!clasp || !(clasp->flags & JSCLASS_IS_ANONYMOUS)) {
    ~^~~~~
    ( == nullptr)
1095

Warning: Implicit conversion 'js::NativeObject *' -> bool [clang-tidy: readability-implicit-bool-conversion]

return GlobalObject::getOrCreateObjectPrototype(cx, global);
       ^                                                   ~
                                                            != nullptr
1106

Warning: Implicit conversion 'JSObject *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!maybeObj || !maybeObj->staticPrototype()) {
                 ~^~~~~~~~                   ~
                 (                            == nullptr)
1157

Warning: Implicit conversion 'unsigned int' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (clasp->flags & JSCLASS_IS_ANONYMOUS) {
    ^~~~~                              ~
    (                                  ) != 0u
1238

Warning: Implicit conversion 'JSObject *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!proto) {
    ~^~~~~~
     == nullptr
1295

Warning: Implicit conversion 'js::LexicalEnvironmentObject *' -> bool [clang-tidy: readability-implicit-bool-conversion]

return obj->is<GlobalObject>() || ObjectRealm::get(obj).getNonSyntacticLexicalEnvironment(obj);
                                  ^~~~~~~~~~~                                                 ~
                                  (                                                            != nullptr)
1316

Warning: Implicit conversion 'JS::Realm *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!cx->realm()) {
    ~^~         ~
                 == nullptr
1712

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

"\"string\", \"number\", or \"default\"",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
R"("string", "number", or "default")"
1753

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

"\"string\", \"number\", or \"default\"", source);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
R"("string", "number", or "default")"
1794

Warning: Implicit conversion 'JS::CallArgs *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (args) {
    ^   ~
         != nullptr
2211

Warning: Implicit conversion 'unsigned int' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (attrs & (JSPROP_GETTER | JSPROP_SETTER)) {
    ^~~~~                                  ~
    (                                      ) != 0u
2232

Warning: Implicit conversion 'JSNative' (aka 'bool (*)(JSContext *, unsigned int, JS::Value *)') -> bool [clang-tidy: readability-implicit-bool-conversion]

if (get.op) {
    ^     ~
           != nullptr
2238

Warning: Implicit conversion 'JSFunction *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!getter) {
    ~^~~~~~~
     == nullptr
2250

Warning: Implicit conversion 'JSNative' (aka 'bool (*)(JSContext *, unsigned int, JS::Value *)') -> bool [clang-tidy: readability-implicit-bool-conversion]

if (set.op) {
    ^     ~
           != nullptr
2256

Warning: Implicit conversion 'JSFunction *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!setter) {
    ~^~~~~~~
     == nullptr
2260

Warning: Implicit conversion 'const JSJitInfo *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (set.info) {
    ^       ~
             != nullptr
3196

Warning: Implicit conversion 'JSAtom *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!name) {
    ~^~~~~
     == nullptr
3586

Warning: Implicit conversion 'const char *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (name) {
    ^   ~
         != nullptr
3593

Warning: Implicit conversion 'unsigned int' -> bool [clang-tidy: readability-implicit-bool-conversion]

return (flags & JSFUN_CONSTRUCTOR)
       ^
                                   != 0u
3612

Warning: Implicit conversion 'JSAtom *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!shAtom) {
    ~^~~~~~~
     == nullptr
3662

Warning: Implicit conversion 'unsigned int' -> bool [clang-tidy: readability-implicit-bool-conversion]

} else if (fs->flags & JSFUN_CONSTRUCTOR) {
           ^~                           ~
           (                            ) != 0u
3667

Warning: Implicit conversion 'JSFunction *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!fun) {
    ~^~~~
     == nullptr
3716

Warning: Implicit conversion 'JSScript *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!JSFunction::getOrCreateScript(cx, fun)) {
    ~^~~~~~~~~~                            ~
                                            == nullptr
3968

Warning: Implicit conversion 'char *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!copy) {
    ~^~~~~
     == nullptr
3995

Warning: Implicit conversion 'const char16_t *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (s) {
    ^~
      != nullptr
4117

Warning: Implicit conversion 'JSScript *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!script) {
    ~^~~~~~~
     == nullptr
4134

Warning: Implicit conversion 'JSFunction *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (fun) {
    ^  ~
        != nullptr
4192

Warning: Implicit conversion 'JSObject *' -> bool [clang-tidy: readability-implicit-bool-conversion]

return !!module;
        ~^~~~~~~
        ( == nullptr)
4364

Warning: Implicit conversion 'JSObject *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!executor) {
    ~^~~~~~~~~
     == nullptr
4542

Warning: Implicit conversion 'JSObject *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!unwrappedPromiseObj) {
    ~^~~~~~~~~~~~~~~~~~~~
     == nullptr
5062

Warning: Implicit conversion 'const char *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!s) {
    ~^~
     == nullptr
5175

Warning: Implicit conversion 'size_t' (aka 'unsigned long') -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!n) {
    ~^~
     == 0u
5249

Warning: Implicit conversion 'JSLinearString *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!linear) {
    ~^~~~~~~
     == nullptr
5380

Warning: Implicit conversion 'JSLinearString *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!linearStr) {
    ~^~~~~~~~~~
     == nullptr
5624

Warning: Statement should be inside braces [clang-tidy: readability-braces-around-statements]

StringifyBehavior::RestrictedSafe))
                                   ^
                                    {
6033

Warning: Implicit conversion 'JSLinearString *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (!input) {
    ~^~~~~~
     == nullptr
6078

Warning: Implicit conversion bool -> 'unsigned int' [clang-tidy: readability-implicit-bool-conversion]

return false;
       ^~~~~~
       0u
6235

Warning: Implicit conversion 'JSExceptionState *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (state) {
    ^    ~
          != nullptr
6289

Warning: Implicit conversion 'const char16_t *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (ownsLinebuf_ && linebuf_) {
                    ^~~~~~~~~
                    ( != nullptr)
6599

Warning: Implicit conversion 'uint32_t' (aka 'unsigned int') -> bool [clang-tidy: readability-implicit-bool-conversion]

jit::JitOptions.disableOptimizationTracking = !value;
                                              ~^~~~~~
                                              ( == 0u)
6603

Warning: Implicit conversion 'uint32_t' (aka 'unsigned int') -> bool [clang-tidy: readability-implicit-bool-conversion]

jit::JitOptions.enableTraceLogger = !!value;
                                     ~^~~~~~
                                     ( == 0u)
6607

Warning: Implicit conversion 'uint32_t' (aka 'unsigned int') -> bool [clang-tidy: readability-implicit-bool-conversion]

jit::JitOptions.spectreIndexMasking = !!value;
                                       ~^~~~~~
                                       ( == 0u)
6625

Warning: Implicit conversion 'uint32_t' (aka 'unsigned int') -> bool [clang-tidy: readability-implicit-bool-conversion]

jit::JitOptions.wasmFoldOffsets = !!value;
                                   ~^~~~~~
                                   ( == 0u)
6670

Warning: Implicit conversion bool -> 'uint32_t' (aka 'unsigned int') [clang-tidy: readability-implicit-bool-conversion]

*valueOut = rt->canUseOffthreadIonCompilation();
            ^~                                 ~
            static_cast<uint32_t>(             )
6750

Warning: Implicit conversion 'js::ScriptSource *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (ss_) {
    ^  ~
        != nullptr
6802

Warning: Implicit conversion 'unsigned int *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (column) {
    ^     ~
           != nullptr
7202

Warning: Implicit conversion 'bool *' -> bool [clang-tidy: readability-implicit-bool-conversion]

if (addr) {
    ^   ~
         != nullptr
toolkit/components/telemetry/build_scripts/gen_event_data.py
41

Error: At least two spaces before inline comment [flake8: E261]

janx added a comment.Oct 12 2018, 1:06 PM

Code analysis found 5 defects in this patch:

  • 5 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -p path/to/file.cpp (C/C++)

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply): https://queue.taskcluster.net/v1/task/local instance/runs/0/artifacts/public/results/PHID-DIFF-62qfefgct52px4d7wwby-clang-format.diff

If you see a problem in this automated review, please report it here: https://bit.ly/2IyNRy2

.arcconfig
2

Such comment, wow

dom/media/CubebUtils.cpp
685–686

Warning: Incorrect coding style [clang-format]

js/ductwork/debugger/JSDebugger.cpp
54–57

Warning: Incorrect coding style [clang-format]

js/src/jsapi.cpp
125–132

Warning: Incorrect coding style [clang-format]

141–147

Warning: Incorrect coding style [clang-format]

233–234

Warning: Incorrect coding style [clang-format]

janx updated this revision to Diff 1204.Oct 12 2018, 1:29 PM
Harbormaster completed remote builds in B921: Diff 1204.Oct 12 2018, 1:29 PM
ReviewBot added a comment.Oct 12 2018, 1:48 PM

Code analysis found 21 defects in this patch:

  • 13 defects found by clang-tidy
  • 8 defects found by mozlint

You can run this analysis locally with:

  • ./mach static-analysis check path/to/file.cpp (C/C++)
  • ./mach lint path/to/file (JS/Python/etc)

If you see a problem in this automated review, please report it here: https://bit.ly/2IyNRy2

browser/base/content/docs/tabbrowser/index.rst
7

Error: Exisits ==> exists [codespell]

devtools/client/definitions.js
7

Error: Infix operators must be spaced. [eslint: space-infix-ops]

devtools/client/menus.js
41

Error: Unexpected tab character. [eslint: no-tabs]

devtools/client/shared/scroll.js
7

Error: Parsing error: Unexpected character '#' [eslint]

dom/media/CubebUtils.cpp
685

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

} else if (aPreferred == CUBEB_DEVICE_PREF_ALL) { // before: readability-else-after-return
  ^~~~~~~~
js/ductwork/debugger/JSDebugger.cpp
50

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

else if (!JS_IsGlobalObject(obj)) { // after: readability-else-after-return
^~~~~~~~
js/src/jsapi.cpp
130

Warning: Suspicious string literal, probably missing a comma [clang-tidy: bugprone-suspicious-missing-comma]

"line 6"     // after: bugprone-suspicious-missing-comma
^
142

Error: Methods annotated with MOZ_NO_DANGLING_ON_TEMPORARIES cannot be && ref-qualified [clang-tidy: mozilla-dangling-on-temporary]

MOZ_NO_DANGLING_ON_TEMPORARIES int *get() && { return nullptr; } // after: mozilla-dangling-on-temporary
                                    ^
143

Error: Methods annotated with MOZ_NO_DANGLING_ON_TEMPORARIES must return a pointer [clang-tidy: mozilla-dangling-on-temporary]

MOZ_NO_DANGLING_ON_TEMPORARIES int test() { return 0; } // after: mozilla-dangling-on-temporary
                                   ^
195

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

else if (ErrorTakesArguments(code_)) { // after: readability-misleading-indentation or readability-else-after-return
^~~~~~~~
227

Warning: Variable 'x' with floating point type 'float' should not be used as a loop counter [clang-tidy: clang-analyzer-security.FloatLoopCounter]

for (float x = 0.1f; x <= 1.0f; x += 0.1f) {} // after: clang-analyzer-security.FloatLoopCounter
^
229

Warning: The return value from the call to 'setuid' is not checked. If an error occurs in 'setuid', the following code may execute with unexpected privileges [clang-tidy: clang-analyzer-security.insecureAPI.UncheckedReturn]

setuid(1); // after: clang-analyzer-security.insecureAPI.UncheckedReturn
^
233

Warning: Potentially unintended semicolon [clang-tidy: bugprone-suspicious-semicolon]

if (x < y); // after: bugprone-suspicious-semicolon; readability-braces-around-statements
          ^
233

Warning: Statement should be inside braces [clang-tidy: readability-braces-around-statements]

if (x < y); // after: bugprone-suspicious-semicolon; readability-braces-around-statements
          ^
           {
251

Warning: Value stored to 'x' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

x = 1; // after: clang-analyzer-deadcode.DeadStores
^
356

Warning: 'find' called with a string literal consisting of a single character; consider using the more effective overload accepting a character [clang-tidy: performance-faster-string-find]

str.find("A"); // after: performance-faster-string-find
         ^~~~
         'A'
365

Warning: The type of the loop variable 'p' is different from the one returned by the iterator and generates an implicit conversion; you can either change the type to the matching one ('const std::pair<const int, std::vector<std::cxx11::basic_string<char>, std::allocator<std::cxx11::basic_string<char> > > > &' but 'const auto&' is always a valid option) or remove the reference to make it explicit that you are creating a new value [clang-tidy: performance-implicit-conversion-in-loop]

for (const std::pair<int, std::vector<std::string>>& p : my_map) {} // after: performance-implicit-conversion-in-loop
     ^
toolkit/components/telemetry/build_scripts/gen_event_data.py
41

Error: Missing whitespace around operator [flake8: E225]

41

Error: Line too long (177 > 99 characters) [flake8: E501]

41

Error: At least two spaces before inline comment [flake8: E261]

65

Error: Continuation line under-indented for visual indent [flake8: E128]

janx added a comment.Oct 12 2018, 1:57 PM

Code analysis found 3 defects in this patch:

  • 3 defects found by clang-format

You can run this analysis locally with:

  • ./mach clang-format -p path/to/file.cpp (C/C++)

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply): https://queue.taskcluster.net/v1/task/local instance/runs/0/artifacts/public/results/PHID-DIFF-lqoyg2yd4b3jfkjkupni-clang-format.diff

If you see a problem in this automated review, please report it here: https://bit.ly/2IyNRy2

js/ductwork/debugger/JSDebugger.cpp
54–57

Warning: Incorrect coding style [clang-format]

js/src/jsapi.cpp
125–131

Warning: Incorrect coding style [clang-format]

141–142

Warning: Incorrect coding style [clang-format]

Revision Contents
Changeset List

PathSize
browser/
base/
content/
docs/
tabbrowser/
devtools/
client/
shared/
dom/
media/
js/
ductwork/
debugger/
src/
toolkit/
components/
telemetry/
build_scripts/

Diff 1155

View Options

.arcconfig

Loading...
View Options

browser/base/content/docs/tabbrowser/index.rst

Loading...
View Options

devtools/client/definitions.js

Loading...
View Options

devtools/client/menus.js

Loading...
View Options

devtools/client/shared/scroll.js

Loading...
View Options

dom/media/CubebUtils.cpp

Loading...
View Options

js/ductwork/debugger/JSDebugger.cpp

Loading...
View Options

js/src/jsapi.cpp

Loading...
View Options

toolkit/components/telemetry/build_scripts/gen_event_data.py

Loading...
Privacy · Cookies · Legal