1629756 - clang-format adds extra indent when [[nodiscard]] function attribute follows NS_DECL_* macro block

Status: NEW

(Developer Infrastructure :: Lint and Formatting, defect, P3)

Description

[Chris Peterson [:cpeterson]]

In meta bug 1571631, I am replacing the MOZ_MUST_USE macro with C++17's standard [[nodiscard]] attribute. Unfortunately, clang-format gets confused when the [[nodiscard]] attribute precedes a function declaration that follows a block of NS_DECL_* macros that have no trailing semicolons. (The macros don't need trailing semicolons because their macro definitions end with a semicolon.)

So replacing MOZ_MUST_USE with [[nodiscard]] in the following example:

class FooWidget : public nsBaseWidget {
public:
  FooWidget();

  NS_DECL_ISUPPORTS_INHERITED

  MOZ_MUST_USE nsresult FunctionOne();
  MOZ_MUST_USE nsresult FunctionTwo();
};

causes clang-format to indent FunctionOne() an extra level like:

class FooWidget : public nsBaseWidget {
public:
  FooWidget();

  NS_DECL_ISUPPORTS_INHERITED

    [[nodiscard]] nsresult FunctionOne();
  [[nodiscard]] nsresult FunctionTwo();
};

clang-format thinks the [[attribute]] implies NS_DECL_ISUPPORTS_INHERITED is part of DoSomething()'s function declaration and thus indents what it thinks is a long function declaration continued over multiple lines. (The [[attribute]] can be any string. It doesn't have to be [[nodiscard]].)

Unfortunately, I see no straightforward way to work around this indent problem in .clang-format other than adding a long, ad hoc list of NS_DECL_* macros to .clang-format's StatementMacros list. This will be hard to maintain and won't be discoverable the next time someone adds a new [[nodiscard]] function. If StatementMacros accepted regular expressions (like MacroBlockBegin/End do), I could just whitelist all NS_DECL_* macros.

I didn't see any clang-format bugs in https://bugs.llvm.org/ related to this [[attribute]] indent problem.

Comment 1

[Chris Peterson [:cpeterson]]

Another workaround is to move NS_DECL_* macro blocks before a function declaration that has no [[attribute]]. Stating in the coding style guide that NS_DECL_* macro blocks must precede the class's constructor would avoid this clang-format problem. It might also make the class definition easier to read if NS_DECL_* macros are consolidate in one block instead of sprinkled throughout the class definition. Win-win?

Comment 2

[Chris Peterson [:cpeterson]]

Sylvestre suggests adding an extra semicolon after the last NS_DECL_* macro instead of using // clang-format off. The extra semicolon allows clang-format to understand that the macro is not part of the [[nodiscard]] function's declaration.

I was reluctant because that extra semicolon would trigger (opt-in) -Wextra-semi-stmt compiler warnings (about useless semicolons because the NS_DECL_* macros already end with a semicolon). However, I doubt we'll ever enable -Wextra-semi-stmt warnings by default because we would have to fix about 4500 -Wextra-semi-stmt warnings in mozilla-central and the value of this warning seems low.

So I will work around these clang-format issues by adding an extra semicolon (and a comment referencing this bug) instead of using // clang-format off.

Comment 3

[Chris Peterson [:cpeterson]]

I filed a clang-format bug: https://bugs.llvm.org/show_bug.cgi?id=45614

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:ahal, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[MyDeveloperDay]

tentative fix https://reviews.llvm.org/D79990