1510682 - clang-format does not keep newlines on ClassOps / ::class_ definitions.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Nicolas B. Pierron [:nbp]]

clang-format, when seeing structure initialization will attempt to pack all the fields as much as possible whereas we splat them for better readability.

ClassOps, ClassSpec and ::class_ definitions are subject to these modification and make things either be strangely organized, and harder to read.

Here are examples taken from builtin/Array.cpp:

before:
> static const ClassOps ArrayObjectClassOps = {
>     array_addProperty,
>     nullptr, /* delProperty */
>     nullptr, /* enumerate */
>     nullptr, /* resolve */
>     nullptr, /* mayResolve */
>     nullptr, /* finalize */
>     nullptr, /* call */
>     nullptr, /* hasInstance */
>     nullptr, /* construct */
>     nullptr, /* trace */
> };

after:
> static const ClassOps ArrayObjectClassOps = {
>     array_addProperty, nullptr, /* delProperty */
>     nullptr,                    /* enumerate */
>     nullptr,                    /* resolve */
>     nullptr,                    /* mayResolve */
>     nullptr,                    /* finalize */
>     nullptr,                    /* call */
>     nullptr,                    /* hasInstance */
>     nullptr,                    /* construct */
>     nullptr,                    /* trace */
> };

We notice that the lack of comments after array_addProperty make clang-format wrap the next field initialization to the previous line. This might be worked around by adding extra /* addProperty */.

It might also be interesting to test adding these comments as prefix.


before:
> const Class ArrayObject::class_ = {
>     "Array",
>     JSCLASS_HAS_CACHED_PROTO(JSProto_Array) | JSCLASS_DELAY_METADATA_BUILDER,
>     &ArrayObjectClassOps,
>     &ArrayObjectClassSpec
> };

after:
> const Class ArrayObject::class_ = {
>     "Array",
>     JSCLASS_HAS_CACHED_PROTO(JSProto_Array) | JSCLASS_DELAY_METADATA_BUILDER,
>     &ArrayObjectClassOps, &ArrayObjectClassSpec};

In this case, the last 2 fields are folded and the closing brace is put on the same line as the last field.  I do not know what might be a good work-around but it really feels more obscure to read.

Maybe we should just take the habit of adding a comment to each field of a structure initialization.

Comment 1

[Ted Campbell [:tcampbell]]

I'd support adding the /* addProperty */ comment even for non-nullptr entries. For the second issue I don't have any good ideas, but am also not too bothered by the default.

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: Bug 1510682 - Clang format js/src/builtin/streams. r?jandem

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: Bug 1510682 - Add field comments for uses of JSClassOps. r?jandem

Depends on D58493

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Bug 1510682 - Add field comments for uses of js::ClassExtension. r?jandem

Depends on D58494

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: Bug 1510682 - Add field comments for uses of js::ObjectOps. r?jandem

Depends on D58495

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef29fb21bdd7
Clang format js/src/builtin/streams. r=jandem
https://hg.mozilla.org/integration/autoland/rev/72f4ca2286aa
Add field comments for uses of JSClassOps. r=jandem
https://hg.mozilla.org/integration/autoland/rev/9a1bd82aed93
Add field comments for uses of js::ClassExtension. r=jandem
https://hg.mozilla.org/integration/autoland/rev/8b5ffa9c5266
Add field comments for uses of js::ObjectOps. r=jandem

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ef29fb21bdd7
https://hg.mozilla.org/mozilla-central/rev/72f4ca2286aa
https://hg.mozilla.org/mozilla-central/rev/9a1bd82aed93
https://hg.mozilla.org/mozilla-central/rev/8b5ffa9c5266