1624786 - Replace MOZ_MUST_USE with [[nodiscard]] in widget

Status: RESOLVED FIXED

(Core :: Widget, task, P3)

Description

[Chris Peterson [:cpeterson]]

The MOZ_MUST_USE macro is defined as clang's and gcc's nonstandard attribute((warn_unused_result)). Now that we compile as C++17 by default (bug 1560664), we can replace MOZ_MUST_USE with C++17's standard [[nodiscard]] attribute.

Also move MOZ_MUST_USE before function declarations' specifiers and return type. While clang and gcc's __attribute__((warn_unused_result)) can appear before, between, or after function specifiers and return types, the [[nodiscard]] attribute must precede the function specifiers.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1624786 - Replace MOZ_MUST_USE with [[nodiscard]] in widget and widget/windows. r?jmathies

Also move MOZ_MUST_USE before function declarations' specifiers and return type. While clang and gcc's attribute((warn_unused_result)) can appear before, between, or after function specifiers and return types, the [[nodiscard]] attribute must precede the function specifiers.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1624786 - Replace MOZ_MUST_USE with [[nodiscard]] in widget/android. r?snorp

Also move MOZ_MUST_USE before function declarations' specifiers and return type. While clang and gcc's attribute((warn_unused_result)) can appear before, between, or after function specifiers and return types, the [[nodiscard]] attribute must precede the function specifiers.

Depends on D68146

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1624786 - Replace MOZ_MUST_USE with [[nodiscard]] in widget/cocoa. r?spohl

Also move MOZ_MUST_USE before function declarations' specifiers and return type. While clang and gcc's attribute((warn_unused_result)) can appear before, between, or after function specifiers and return types, the [[nodiscard]] attribute must precede the function specifiers.

Depends on D68147

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Bug 1624786 - Replace MOZ_MUST_USE with [[nodiscard]] in widget/gtk. r?karlt

Also move MOZ_MUST_USE before function declarations' specifiers and return type. While clang and gcc's attribute((warn_unused_result)) can appear before, between, or after function specifiers and return types, the [[nodiscard]] attribute must precede the function specifiers.

Depends on D68148

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: Bug 1624786 - Replace MOZ_MUST_USE with [[nodiscard]] in widget/headless. r?bdahl

Also move MOZ_MUST_USE before function declarations' specifiers and return type. While clang and gcc's attribute((warn_unused_result)) can appear before, between, or after function specifiers and return types, the [[nodiscard]] attribute must precede the function specifiers.

Depends on D68149

Comment 6

[Chris Peterson [:cpeterson]]

leave-open because I'm landing patches #1 and #2 so they don't bitrot while waiting for feedback on patch #3.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/beb748e23620
Replace MOZ_MUST_USE with [[nodiscard]] in widget and widget/windows. r=jmathies
https://hg.mozilla.org/integration/autoland/rev/b21157048591
Replace MOZ_MUST_USE with [[nodiscard]] in widget/android. r=snorp

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/beb748e23620
https://hg.mozilla.org/mozilla-central/rev/b21157048591

Comment 9

[Chris Peterson [:cpeterson]]

nit: The indent seems off here.

Stephen, following up on your comment in https://phabricator.services.mozilla.com/D68148#inline-404257. Unfortunately, hg's clang-format hook forces this indentation (unless I add a superfluous semicolon after NS_DECL_NSPIWIDGETCOCOA).

Which of these options do you prefer?

• Leave clang-format's extra indentation level.
• Add a superfluous semicolon after NS_DECL_NSPIWIDGETCOCOA, though that might trigger a compiler warning about the extra semicolon. (The NS_DECL_NSPIWIDGETCOCOA macro definition already includes a semicolon.)
• Wrap this Create() function declaration in // clang-format off and // clang-format on to disable clang-format for that block of code.

Comment 10

[Chris Peterson [:cpeterson]]

I'm plan to:

1. Wrap the Create() function declaration in // clang-format off and // clang-format on to disable clang-format for that block of code.
2. Ask the linting team whether this is clang-format bug or setting we can fix.

Comment 11

[Chris Peterson [:cpeterson]]

Sylvetre, is the problem below a clang-format bug or setting we can work around?

clang-format insists on indenting the first Create() function declaration here:

https://phabricator.services.mozilla.com/D68148#change-ItZetrh2GZ7W

class nsCocoaWindow final : public nsBaseWidget, public nsPIWidgetCocoa {
...
  NS_DECL_ISUPPORTS_INHERITED
  NS_DECL_NSPIWIDGETCOCOA

    [[nodiscard]] virtual nsresult Create(nsIWidget* aParent, nsNativeWidget aNativeParent,
                                          const DesktopIntRect& aRect,
                                          nsWidgetInitData* aInitData = nullptr) override;

  [[nodiscard]] virtual nsresult Create(nsIWidget* aParent, nsNativeWidget aNativeParent,
                                        const LayoutDeviceIntRect& aRect,
                                        nsWidgetInitData* aInitData = nullptr) override;

clang-format mistakenly thinks the NS_DECL_NSPIWIDGETCOCOA macro a couple lines before the [[nodiscard]] should be part of the Create() function declaration. The NS_DECL_NSPIWIDGETCOCOA macro injects some function declarations should not impact the indentation level of surrounding code. I feel this is a clang-format bug.

https://searchfox.org/mozilla-central/source/__GENERATED__/dist/include/nsPIWidgetCocoa.h#52-57

To work around this, I have to add:

class nsCocoaWindow final : public nsBaseWidget, public nsPIWidgetCocoa {
...
  // clang-format off
  NS_DECL_ISUPPORTS_INHERITED
  NS_DECL_NSPIWIDGETCOCOA

  [[nodiscard]] virtual nsresult Create(nsIWidget* aParent, nsNativeWidget aNativeParent,
                                        const DesktopIntRect& aRect,
                                        nsWidgetInitData* aInitData = nullptr) override;
  // clang-format on

  [[nodiscard]] virtual nsresult Create(nsIWidget* aParent, nsNativeWidget aNativeParent,
                                        const LayoutDeviceIntRect& aRect,
                                        nsWidgetInitData* aInitData = nullptr) override;

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81e4b5376ea8
Replace MOZ_MUST_USE with [[nodiscard]] in widget/cocoa. r=spohl
https://hg.mozilla.org/integration/autoland/rev/c4832a04688a
Replace MOZ_MUST_USE with [[nodiscard]] in widget/gtk. r=karlt
https://hg.mozilla.org/integration/autoland/rev/9e2d7aa1942c
Replace MOZ_MUST_USE with [[nodiscard]] in widget/headless. r=bdahl

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/81e4b5376ea8
https://hg.mozilla.org/mozilla-central/rev/c4832a04688a
https://hg.mozilla.org/mozilla-central/rev/9e2d7aa1942c

Comment 14

[Chris Peterson [:cpeterson]]

Attachment: Bug 1624786 - Replace "clang-format off" with semicolon workaround for clang-format bug 1629756. r?spohl

This semicolon workaround is tidier than the // clang-format off/on comments and avoids turning off all clang-format checks. The comment also links to clang-format bug 1629756 so future code readers can learn why this extra semicolon exists. And if we find a way to fix this in clang-format, then we can search for this bug number to find and remove these extra semicolons and comments.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6637962605f
Replace "clang-format off" with semicolon workaround for clang-format bug 1629756. r=spohl

Comment 16

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a6637962605f

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Sorry for the latency, I think it is now fixed upstream