1559184 - clang-format totally screwed up the formatting in DOMMatrix.h

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Relevant changeset: https://searchfox.org/mozilla-central/diff/265e6721798a455604328ed5262f430cfcc37c2f/dom/base/DOMMatrix.h#75

The new formatting is completely unreadable; it took me a bit to figure out what the heck happened and what the code is doing.

Can we fix this, please?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Also wondering whether there's any way to query for other instances of this...

Comment 2

[Mike Hommey [:glandium]]

Changing the macro calls so that they can be followed by a ;, and adding those ; should make clang-format behave better.

Comment 3

[(no longer active)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #0)

Relevant changeset: https://searchfox.org/mozilla-central/diff/265e6721798a455604328ed5262f430cfcc37c2f/dom/base/DOMMatrix.h#75

The new formatting is completely unreadable; it took me a bit to figure out what the heck happened and what the code is doing.

My apologies Boris, I spent a lot of time looking over the changes to find examples of unintended consequences like this but this one fell through the cracks. What confused clang-format's tokenizer here are all of these macros used with no semi-colon to be found anywhere, which makes a big block of code here look like a word salad to the tokenizer.

Can we fix this, please?

Patch coming up right away.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)

Also wondering whether there's any way to query for other instances of this...

I've thought about it but haven't thought of a good way. Perhaps one could do so using clang-query but I can't say I've used that tool in the past. :-/

Comment 4

[(no longer active)]

Attachment: Bug 1559184 - Fix the damage caused by clang-format to DOMMatrix.h;

Comment 5

[Boris Zbarsky [:bzbarsky]]

The macro calls are declaring member functions. I guess we could put the { and } outside the macro and add begin/end macro stuff to allow the ;... or even not; if the line ends with } clang-format leaves it alone instead of pulling up stuff from the next line, looks like.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ehsan, no need to apologize. I know you spent a lot of time dealing with these issues; there was just a very big haystack for all the needles to hide in.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

This is overkill but StatementMacros might help for similar cases:
https://searchfox.org/mozilla-central/source/.clang-format#28

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e148406af28
Fix the damage caused by clang-format to DOMMatrix.h; r=bzbarsky

Comment 9

[(no longer active)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5)

The macro calls are declaring member functions. I guess we could put the { and } outside the macro and add begin/end macro stuff to allow the ;... or even not; if the line ends with } clang-format leaves it alone instead of pulling up stuff from the next line, looks like.

Yeah, clang-format has some imperfect heuristics around what determines when a line should end, things like ; or } signal that. If you think about the problem clang-format is trying to solve, when faced with code like we had in DOMMatrix.h, it wouldn't have a good way to tell where the line ending should be, so for all it could have said, everything was meant to have been on the same line!

(In reply to Sylvestre Ledru [:sylvestre] from comment #7)

This is overkill but StatementMacros might help for similar cases:
https://searchfox.org/mozilla-central/source/.clang-format#28

Indeed it would. To expand a bit, by default clang-format assumes macros are expanded to be expressions. This config option declares macros that are expanded to become full statements of their own. However IMHO for cases like this where there is a localized macro defined in just one file, turning formatting off locally is a better solution than to add more junk to the global config file (of course just an opinion!)

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3e148406af28

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This grafts cleanly to Beta. Should we uplift it there so ESR68 doesn't have to carry the old formatting over its lifespan?

Comment 12

[Sylvestre Ledru [:Sylvestre]]

However IMHO for cases like this where there is a localized macro defined in just one file, turning formatting off locally is a better solution than to
add more junk to the global config file (of course just an opinion!)

I do agree, I was explaining about this options for people who don't know that it exists :)

Comment 13

[(no longer active)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

This grafts cleanly to Beta. Should we uplift it there so ESR68 doesn't have to carry the old formatting over its lifespan?

Sounds like a good idea.

Comment 14

[(no longer active)]

Comment on attachment 9071987 [details]
Bug 1559184 - Fix the damage caused by clang-format to DOMMatrix.h;

Beta/Release Uplift Approval Request

• User impact if declined: Please see comment 12. This doesn't impact users but it will simplify the lives of our developers while creating uplifts for security fixes on ESR68.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Whitespace only change
• String changes made/needed: None

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9071987 [details]
Bug 1559184 - Fix the damage caused by clang-format to DOMMatrix.h;

approved for 68.0b12

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e43bd705a681