1371052 - Use nullptr and auto specifier in /js/src (clang-tidy modernize-use-auto and modernize-use-nullptr)

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement, P5)

Description

[Jan Keromnes [:janx]]

When running clang-tidy on /js/src with the following command:

    run-clang-tidy-4.0.py -p obj-x86_64-pc-linux-gnu/ -checks=-*,modernize-use-auto,modernize-use-nullptr js/src

We find a few suggestions to improve our code by migrating to C++11. Clang-tidy can even apply them automatically with the `-fix` parameter.

Let's use this bug to make small improvements to the codebase here and there by keeping a small footprint and limit the review burden.

Note: You can install run-clang-tidy-4.0.py on Ubuntu/Debian by following these steps:

    apt-get install clang-tidy-4.0 # Follow http://apt.llvm.org/
    ./mach configure
    ./mach build-backend --backend=CompileDB
    ./mach build pre-export
    ./mach build export
    run-clang-tidy-4.0.py -p obj-*/ -checks=-*,modernize-use-auto,modernize-use-nullptr js/src

Comment 1

[Jan Keromnes [:janx]]

Attachment: Use auto specifier in /js/src/builtin/RegExp.cpp. r=

Comment 2

[Jan Keromnes [:janx]]

Attachment: Use auto specifier in /js/src/ctypes/CTypes.cpp. r=

Comment 3

[Jan Keromnes [:janx]]

Attachment: Use auto specifier in /js/src/ctypes/CTypes.cpp. r=

Comment 4

[Jan Keromnes [:janx]]

Attachment: Use auto specifier in /js/src/gc/StoreBuffer.cpp. r=

Comment 5

[Jan Keromnes [:janx]]

Attachment: Use auto specifier in /js/src/jsapi-tests/testAssemblerBuffer.cpp. r=

Comment 6

[Jan Keromnes [:janx]]

Attachment: Use auto specifier in /js/src/jsatom.cpp. r=

Comment 7

[Jan Keromnes [:janx]]

Attachment: Use nullptr and auto specifier in /js/src/jsdtoa.cpp. r=

Comment 8

[Jan Keromnes [:janx]]

Attachment: user auto specifier in /js/src/vm/TraceLogging.cpp. r=

Comment 9

[Jan Keromnes [:janx]]

Attachment: Use auto specifier in /js/src/frontend/Parser.cpp. r=

Comment 10

[Jan de Mooij [:jandem]]

nullptr is great but I'm not sure auto is an improvement when it changes:

  RegExpFlag flags = RegExpFlag(0);

to:

  auto flags = RegExpFlag(0);

Without auto it's obvious |flags| is a RegExpFlag. With auto we don't know, for instance what if RegExpFlag is a function that returns an integer?

Comment 11

[Jan Keromnes [:janx]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> nullptr is great but I'm not sure auto is an improvement when it changes:
> 
>   RegExpFlag flags = RegExpFlag(0);
> 
> to:
> 
>   auto flags = RegExpFlag(0);
> 
> Without auto it's obvious |flags| is a RegExpFlag. With auto we don't know,
> for instance what if RegExpFlag is a function that returns an integer?

Thanks a lot for confirming one of my worries about the auto specifier! We're considering it as part of our static analysis and C++11 conversion efforts, but if we consider `RegExpFlag(0)` to be a false positive for the `modernize-use-auto` checker, we might decide against using it.

Out of curiosity, do you think other less ambiguous `auto` conversions could be valuable? E.g.:

> -        uint32_t n = (uint32_t)dval(di);
> +        auto n = (uint32_t)dval(di);

> -      CDataFinalizer::Private* p = (CDataFinalizer::Private*)
> -        JS_GetPrivate(sourceData);
> +      auto* p = (CDataFinalizer::Private*)JS_GetPrivate(sourceData);

> -    NativeObject* nproto = static_cast<NativeObject*>(proto);
> +    auto* nproto = static_cast<NativeObject*>(proto);

> -    ImmutablePropertyNamePtr* names = reinterpret_cast<ImmutablePropertyNamePtr*>(commonNames.ref());
> +    auto* names = reinterpret_cast<ImmutablePropertyNamePtr*>(commonNames.ref());

etc.

Comment 12

[Jan Keromnes [:janx]]

Comment on attachment 8875493 [details] [diff] [review]
Use auto specifier in /js/src/builtin/RegExp.cpp. r=

Review of attachment 8875493 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/RegExp.cpp
@@ +219,5 @@
>              return false;
>      }
>  
>      /* Step 3. */
> +    auto flags = RegExpFlag(0);

This use of `auto` makes the code less explicit / understandable.

@@ +456,5 @@
>  
>          // Step 8.
>          if (args.hasDefined(1)) {
>              // Step 4.c / 21.2.3.2.2 RegExpInitialize step 4.
> +            auto flagsArg = RegExpFlag(0);

This use of `auto` makes the code less explicit / understandable.

@@ +1570,5 @@
>      AutoAssertNoPendingException aanpe(cx);
>      if (!proto->isNative())
>          return false;
>  
> +    auto* nproto = static_cast<NativeObject*>(proto);

This use of `auto` might slightly improve the code by making it shorter.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Jan Keromnes [:janx] from comment #11)
> Out of curiosity, do you think other less ambiguous `auto` conversions could
> be valuable? E.g.:

> > -      CDataFinalizer::Private* p = (CDataFinalizer::Private*)
> > -        JS_GetPrivate(sourceData);
> > +      auto* p = (CDataFinalizer::Private*)JS_GetPrivate(sourceData);

Here it could be nice because it's quite a bit shorter.

Personally I prefer seeing the explicit types for each of these cases. That's just my preference and maybe it's because I have to get used to |auto|, but if we're considering rewriting things it might be worth asking dev-platform what people think :)

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

My personal rule of thumb with auto is to use it only when the type is already indicated in the current line, also keeping the pointer/reference annotation (so auto* as in your example). With a few exceptions: declaring iterators over templates structures as auto variables sounds fine to me, to avoid ultra verbose code. (it would make sense indeed to ask dev.platform about this topic)

Comment 15

[Jan Keromnes [:janx]]

Thanks a lot for your feedback!

I'll close this bug for now, because auto specifiers are mostly a coding style preference, and converting is not always a good idea which makes it hard to automate. We'll disable the "modernize-use-auto" clang-tidy rule for now. If we decide to re-enable it in the future with more useful heuristics, we should ask dev.platform about these heuristics.

Comment 16

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.