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?

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

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

(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. :-/

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.

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.

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

(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!)

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

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

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 :)

(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 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 on attachment 9071987 [details]
Bug 1559184 - Fix the damage caused by clang-format to DOMMatrix.h;

approved for 68.0b12

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