Skip to content

[clang-format] AlignAfterOpenBracket: BlockIndent produces inconsistent/bad formatting #55731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
H-G-Hristov opened this issue May 27, 2022 · 10 comments · Fixed by #93140
Closed

Comments

@H-G-Hristov
Copy link
Contributor

H-G-Hristov commented May 27, 2022

Screen Shot 2022-05-27 at 14 30 32

My .clang-format file looks like:

---
BasedOnStyle: Google
Standard: c++20
IndentWidth: 4
ColumnLimit: 120
AccessModifierOffset: -4
AlignAfterOpenBracket: BlockIndent
AlignArrayOfStructures: Right
AllowShortEnumsOnASingleLine: false
AllowShortFunctionsOnASingleLine: Empty
AllowShortLambdasOnASingleLine: Empty
BinPackArguments: false
BinPackParameters: false
BraceWrapping:
  AfterClass: true
  AfterEnum: true
  AfterFunction: true
  AfterStruct: true
  AfterUnion: true
  BeforeLambdaBody: true
  SplitEmptyFunction: false
BreakBeforeBraces: Custom
BreakConstructorInitializers: BeforeComma
IncludeCategories:
  # Third party headers
  - Regex:           '^<(boost|bzip|fmt|grpc|gsl|gtest|gmock|isl|json|openssl|tl|toml\+\+|proto|zlib)/'
    Priority:        3
    SortPriority:    0
    CaseSensitive:   true
  # This library headers
  - Regex:           '^<(nblib)'
    Priority:        4
    SortPriority:    0
    CaseSensitive:   true
  # This library headers
  - Regex:           '^"(nblib)'
    Priority:        5
    SortPriority:    0
    CaseSensitive:   true
  # C system headers
  - Regex:           '^<.+\.h>$'
    Priority:        1
    SortPriority:    0
    CaseSensitive:   true
  # C++ standard library headers
  - Regex:           '^<.+'
    Priority:        2
    SortPriority:    0
    CaseSensitive:   true
  # Other headers
  - Regex:           '^".*'
    Priority:        6
    SortPriority:    0
    CaseSensitive:   true
IndentPPDirectives: AfterHash
PackConstructorInitializers: Never
# Different ways to arrange specifiers and qualifiers (e.g. const/volatile).
QualifierAlignment: Left
# Specifies the use of empty lines to separate definition blocks, including classes, structs, enums, and functions.
SeparateDefinitionBlocks: Always
# Add namespace comment after short namespaces
ShortNamespaceLines: 0
...

The result looks like:

#pragma once

#include <memory>
#include <optional>

#include <nblib/types/IForwardIterator.hpp>
#include <nblib/utilities/IFileSystem.hpp>
#include <nblib/utilities/Logging.hpp>

#include "nblib/data/hardware/IStorage.hpp"

namespace nblib::data {

#if defined(__APPLE__)
class ISystemProfiler;
#endif  // defined(__APPLE__)

}  // namespace nblib::data

namespace nblib::utilities {

#if defined(__linux__)
class IProcessSpawner;
#endif  // defined(__linux__)

}  // namespace nblib::utilities

namespace nblib::data {

using LogicalVolumeIteratorBase = nblib::types::IForwardIterator<IStorage::LogicalVolumeInfo>;

class Storage : public IStorage
{
public:
    Storage();

#if defined(__APPLE__)
    explicit Storage(std::unique_ptr<ISystemProfiler> systemProfiler);
#endif

#if defined(__linux__)
    explicit Storage(std::unique_ptr<::nblib::utilities::IProcessSpawner> processSpawner);
#endif

public:
    std::vector<LogicalVolumeInfo> GetLogicalVolumes() const noexcept override;

    std::vector<LogicalVolumeInfo> GetNonSystemLogicalVolumes() const noexcept override;

    std::vector<LogicalVolumeInfo> GetInternalNonRemovableLogicalVolumes() const noexcept override;

    std::vector<std::string> GetPhysicalDiskDriveNames() const noexcept override;

    std::vector<std::string> GetInternalNonRemovablePhysicalDiskDriveNames() const noexcept override;

    std::vector<OpticalDiscDriveInfo> GetOpticalDiscDrives() const noexcept override;

    std::vector<std::string> GetFloppyDiskDriveNames() const noexcept override;

    std::optional<PhysicalDiskDriveInfo> GetPhysicalDiskDriveInfo(const std::string& deviceName
    ) const noexcept override;

    std::optional<LogicalVolumeInfo> GetLogicalVolumeInfo(const std::string& deviceName) const noexcept override;

private:
    template <typename Iterator>
    std::optional<LogicalVolumeInfo> GetLogicalVolumeInfo(Iterator&& iterator, const std::string& deviceName)
        const noexcept;

private:
#if defined(__APPLE__)
    std::unique_ptr<ISystemProfiler> m_systemProfiler;
#endif  // __APPLE__

#if defined(__linux__)
    std::unique_ptr<::nblib::utilities::IProcessSpawner> m_processSpawner;
#endif  // __linux__
};

template <typename IteratorT>
std::optional<IStorage::LogicalVolumeInfo> Storage::GetLogicalVolumeInfo(
    IteratorT&& iterator, const std::string& deviceName
) const noexcept
{
    static_assert(std::is_base_of_v<LogicalVolumeIteratorBase, IteratorT>);

    try {
        LogicalVolumeInfo volumeInfo{};
        while (iterator.GetNext(volumeInfo)) {
            if (volumeInfo.deviceName == deviceName) {
                return volumeInfo;
            }
        }

        return {};
    } catch (const std::exception& e) {
        UCLOG_ERROR << "Unable to get logical volume info: " << e.what();

        return {};
    }
}

}  // namespace nblib::data

I would expect something like:

#pragma once

#include <memory>
#include <optional>

#include <nblib/types/IForwardIterator.hpp>
#include <nblib/utilities/IFileSystem.hpp>
#include <nblib/utilities/Logging.hpp>

#include "nblib/data/hardware/IStorage.hpp"

namespace nblib::data {

#if defined(__APPLE__)
class ISystemProfiler;
#endif  // defined(__APPLE__)

}  // namespace nblib::data

namespace nblib::utilities {

#if defined(__linux__)
class IProcessSpawner;
#endif  // defined(__linux__)

}  // namespace nblib::utilities

namespace nblib::data {

using LogicalVolumeIteratorBase = nblib::types::IForwardIterator<IStorage::LogicalVolumeInfo>;

class Storage : public IStorage
{
public:
    Storage();

#if defined(__APPLE__)
    explicit Storage(std::unique_ptr<ISystemProfiler> systemProfiler);
#endif

#if defined(__linux__)
    explicit Storage(std::unique_ptr<::nblib::utilities::IProcessSpawner> processSpawner);
#endif

public:
    std::vector<LogicalVolumeInfo> GetLogicalVolumes() const noexcept override;

    std::vector<LogicalVolumeInfo> GetNonSystemLogicalVolumes() const noexcept override;

    std::vector<LogicalVolumeInfo> GetInternalNonRemovableLogicalVolumes() const noexcept override;

    std::vector<std::string> GetPhysicalDiskDriveNames() const noexcept override;

    std::vector<std::string> GetInternalNonRemovablePhysicalDiskDriveNames() const noexcept override;

    std::vector<OpticalDiscDriveInfo> GetOpticalDiscDrives() const noexcept override;

    std::vector<std::string> GetFloppyDiskDriveNames() const noexcept override;

    std::optional<PhysicalDiskDriveInfo> GetPhysicalDiskDriveInfo(
       const std::string& deviceName
    ) const noexcept override;

    std::optional<LogicalVolumeInfo> GetLogicalVolumeInfo(const std::string& deviceName) const noexcept override;

private:
    template <typename Iterator>
    std::optional<LogicalVolumeInfo> GetLogicalVolumeInfo(
        Iterator&& iterator, const std::string& deviceName
    ) const noexcept;

private:
#if defined(__APPLE__)
    std::unique_ptr<ISystemProfiler> m_systemProfiler;
#endif  // __APPLE__

#if defined(__linux__)
    std::unique_ptr<::nblib::utilities::IProcessSpawner> m_processSpawner;
#endif  // __linux__
};

template <typename IteratorT>
std::optional<IStorage::LogicalVolumeInfo> Storage::GetLogicalVolumeInfo(
    IteratorT&& iterator, const std::string& deviceName
) const noexcept
{
    static_assert(std::is_base_of_v<LogicalVolumeIteratorBase, IteratorT>);

    try {
        LogicalVolumeInfo volumeInfo{};
        while (iterator.GetNext(volumeInfo)) {
            if (volumeInfo.deviceName == deviceName) {
                return volumeInfo;
            }
        }

        return {};
    } catch (const std::exception& e) {
        UCLOG_ERROR << "Unable to get logical volume info: " << e.what();

        return {};
    }
}

}  // namespace nblib::data

If a function declaration cannot fit on a single line it should preferably be broken after the opening and closing bracket always.

   ...

    // Fits on a single line:
    void myFunc2(int param) const noexcept;
    
    // Doesn't fit on a single line:
    void myFunc2(
        long long veryLongParametrName
    ) const noexcept;

   void myFunc3(
       int param1,
       int param2,
   ) const noexcept;
   
   // What about trailing return types?
   auto myFunc3(
       int param1,
       int param2,
   ) const noexcept -> std::vector<of_very_long_type>;

   auto myFunc3(
       int param1,
       int param2,
   ) const noexcept 
     -> std::vector<of_very_long_type>;

   ...

I love this new option but it needs work for braces {} and brackets (), etc.

References:

@H-G-Hristov
Copy link
Contributor Author

@csmulhern

@llvmbot
Copy link
Member

llvmbot commented May 27, 2022

@llvm/issue-subscribers-clang-format

@stinos
Copy link

stinos commented Jun 22, 2022

Not sure if related, but there seems to be some strange interaction between using BlockIndent and the ContinuationIndentWidth value. Code:

int LotsOfArgs(int iddddddddddddddddddddddddddddddddddddddd, int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)
{
  const auto a = true || true || true || true || true || true || true || true || true || true || true || true || true || true || true || true || true || true;
  if( true || true || true || true || true || true || true || true || true || true || true || true || true || true || true || true || true || true )
  {}
}

with the settings shown above this gets turned into

int LotsOfArgs(
    int iddddddddddddddddddddddddddddddddddddddd,
    int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
)
{
    const auto a = true || true || true || true || true || true || true || true || true || true || true || true ||
                   true || true || true || true || true || true;
    if (true || true || true || true || true || true || true || true || true || true || true || true || true || true ||
        true || true || true || true) {
    }
}

which is ok. Now I want 2 spaces for indent so change IndentWidth and ContinuationIndentWidth to 2, but then the if isn't broken anymore which isn't expected (more than 120 characters):

int LotsOfArgs(
  int iddddddddddddddddddddddddddddddddddddddd,
  int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
)
{
  const auto a = true || true || true || true || true || true || true || true || true || true || true || true || true ||
                 true || true || true || true || true;
  if (true || true || true || true || true || true || true || true || true || true || true || true || true || true || true || true || true || true) {
  }
}

now change AlignAfterOpenBracket into Align and it breaks again:

int LotsOfArgs(int iddddddddddddddddddddddddddddddddddddddd,
               int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)
{
  const auto a = true || true || true || true || true || true || true || true || true || true || true || true || true ||
                 true || true || true || true || true;
  if (true || true || true || true || true || true || true || true || true || true || true || true || true || true ||
      true || true || true || true) {
  }
}

@martinling
Copy link

Since it seems this is now the chosen issue for tracking this bug, #54808 could be closed as another duplicate.

@madmiraal
Copy link

A simple version of the problem might help:
.clang-format:

AlignAfterOpenBracket: BlockIndent

Before:

// ----------------------------------------------------------- Column Width 80 >|<
bool ClassName::method(const bool &paramter1, const bool &parameter2) const {
  return true;
}

bool ClassName::longMethodName(const bool &paramter1, const bool &parameter2) const {
  return true;
}

bool ClassName::veryVeryLongMethodName(const bool &paramter1, const bool &parameter2) const {
  return true;
}

After:

// Good: Fits on one line
bool ClassName::method(const bool &paramter1, const bool &parameter2) const {
  return true;
}

// Bad: Unexpected!
bool ClassName::longMethodName(const bool &paramter1, const bool &parameter2)
    const {
  return true;
}

// Good: Expected
bool ClassName::veryVeryLongMethodName(
    const bool &paramter1, const bool &parameter2
) const {
  return true;
}

I've tried reducing the PenaltyBreakBeforeFirstCallParameter to 0 (I believe the default is 19) to encourage the break there, but that doesn't help.

.clang-format:

AlignAfterOpenBracket: BlockIndent
PenaltyBreakBeforeFirstCallParameter: 0

@H-G-Hristov
Copy link
Contributor Author

A simple version of the problem might help: .clang-format:

AlignAfterOpenBracket: BlockIndent

Before:

// ----------------------------------------------------------- Column Width 80 >|<
bool ClassName::method(const bool &paramter1, const bool &parameter2) const {
  return true;
}

bool ClassName::longMethodName(const bool &paramter1, const bool &parameter2) const {
  return true;
}

bool ClassName::veryVeryLongMethodName(const bool &paramter1, const bool &parameter2) const {
  return true;
}

After:

// Good: Fits on one line
bool ClassName::method(const bool &paramter1, const bool &parameter2) const {
  return true;
}

// Bad: Unexpected!
bool ClassName::longMethodName(const bool &paramter1, const bool &parameter2)
    const {
  return true;
}

// Good: Expected
bool ClassName::veryVeryLongMethodName(
    const bool &paramter1, const bool &parameter2
) const {
  return true;
}

I've tried reducing the PenaltyBreakBeforeFirstCallParameter to 0 (I believe the default is 19) to encourage the break there, but that doesn't help.

.clang-format:

AlignAfterOpenBracket: BlockIndent
PenaltyBreakBeforeFirstCallParameter: 0

Your examples could also be expanded by placing a single parameter on a line:

void func(
   type1 param1,
   type2 param2,
   ...
) 

@madmiraal
Copy link

Your examples could also be expanded by placing a single parameter on a line:

void func(
   type1 param1,
   type2 param2,
   ...
) 

This is a separate, but related issue i.e. AllowAllArgumentsOnNextLine: false is also not working with AlignAfterOpenBracket: BlockIndent even when setting BinPackArguments and BinPackParameters to false.

Expanded examples:
.clang-format:

AlignAfterOpenBracket: BlockIndent
AllowAllArgumentsOnNextLine: false
BinPackArguments: false
BinPackParameters: false
PenaltyBreakBeforeFirstCallParameter: 0
PenaltyReturnTypeOnItsOwnLine: 1000
class ClassName {
  // Expected: Fits on a single line
  bool method(const bool &parameter1, const bool &parameter2) const;

  // Expected: Fits on a single line
  bool longMethodName(const bool &parameter1, const bool &parameter2) const;

  // Unexpected: `const` on its own.
  bool veryVeryLongMethodName(const bool &parameter1, const bool &parameter2)
      const;

  // Unexpected: All parameters on the next line
  bool veryVeryVeryVeryLongMethodName(
      const bool &parameter1, const bool &parameter2
  ) const;

  // Expected: Each parameter on a separate line.
  bool method(
      const bool &parameter1,
      const bool &parameter2,
      const bool &parameter3,
      const bool &parameter4,
      const bool &parameter5
  ) const;
};

// ----------------------------------------------------------- Column Width 80
// >|<

// Expected: Fits on a single line
bool ClassName::method(const bool &parameter1, const bool &parameter2) const {
  return true;
}

// Unexpected: `const` on its own.
bool ClassName::longMethodName(const bool &parameter1, const bool &parameter2)
    const {
  return true;
}

// Unexpected: All parameters on the next line
bool ClassName::veryVeryLongMethodName(
    const bool &parameter1, const bool &parameter2
) const {
  return true;
}

// Unexpected: All parameters on the next line
bool ClassName::veryVeryVeryVeryLongMethodName(
    const bool &parameter1, const bool &parameter2
) const {
  return true;
}

// Expected: Each parameter on a separate line.
bool ClassName::method(
    const bool &parameter1,
    const bool &parameter2,
    const bool &parameter3,
    const bool &parameter4,
    const bool &parameter5
) const {
  return true;
}

For comparison it works as expected with AlignAfterOpenBracket: Align:

class ClassName {
  bool method(const bool &parameter1, const bool &parameter2) const;

  bool longMethodName(const bool &parameter1, const bool &parameter2) const;

  bool veryVeryLongMethodName(const bool &parameter1,
                              const bool &parameter2) const;

  bool veryVeryVeryVeryLongMethodName(const bool &parameter1,
                                      const bool &parameter2) const;

  bool method(const bool &parameter1,
              const bool &parameter2,
              const bool &parameter3,
              const bool &parameter4,
              const bool &parameter5) const;
};

// ----------------------------------------------------------- Column Width 80
// >|<

bool ClassName::method(const bool &parameter1, const bool &parameter2) const {
  return true;
}

bool ClassName::longMethodName(const bool &parameter1,
                               const bool &parameter2) const {
  return true;
}

bool ClassName::veryVeryLongMethodName(const bool &parameter1,
                                       const bool &parameter2) const {
  return true;
}

bool ClassName::veryVeryVeryVeryLongMethodName(const bool &parameter1,
                                               const bool &parameter2) const {
  return true;
}

bool ClassName::method(const bool &parameter1,
                       const bool &parameter2,
                       const bool &parameter3,
                       const bool &parameter4,
                       const bool &parameter5) const {
  return true;
}

@EmrecanKaracayir
Copy link

EmrecanKaracayir commented Mar 20, 2024

Any updates on this?

Here is how BlockIndent setting results with in my code:

void Workshop02::run()
{
  ANNOUNCE_WORKSHOP("Workshop 02: Uninitialized Variables & Undefined Behavior"
  );
  int x;
  std::cout << "Uninitialized variable x: " << x << '\n';
  std::cout << "Size of an int: " << sizeof(int) << '\n';
}

Expected result:

void Workshop02::run()
{
  ANNOUNCE_WORKSHOP(
    "Workshop 02: Uninitialized Variables & Undefined Behavior"
  );
  int x;
  std::cout << "Uninitialized variable x: " << x << '\n';
  std::cout << "Size of an int: " << sizeof(int) << '\n';
}

@gedare
Copy link
Contributor

gedare commented May 23, 2024

This is a separate, but related issue i.e. AllowAllArgumentsOnNextLine: false is also not working with AlignAfterOpenBracket: BlockIndent even when setting BinPackArguments and BinPackParameters to false.

Expanded examples: .clang-format:

AlignAfterOpenBracket: BlockIndent
AllowAllArgumentsOnNextLine: false
BinPackArguments: false
BinPackParameters: false
PenaltyBreakBeforeFirstCallParameter: 0
PenaltyReturnTypeOnItsOwnLine: 1000

Please retry with AllowAllParametersOfDeclarationOnNextLine: false

gedare added a commit to gedare/llvm-project that referenced this issue May 23, 2024
Fixes llvm#55731
Fixes llvm#73584

The reported formatting problems were related to ignoring deep nesting
of "simple" functions (causing llvm#54808) and to allowing the trailing
annotation to become separated from the closing parens, which allowed a
break to occur between the closing parens and the trailing annotation.
The fix for the nesting of "simple" functions is to detect them more
carefully. "Simple" was defined in a comment as being a single
non-expression argument. I tried to stay as close to the original intent
of the implementation while fixing the various bad formatting reports.

In the process of fixing these bugs, some latent bugs were discovered
related to how JavaScript Template Strings are handled. Those are also
fixed here.
@madmiraal
Copy link

Please retry with AllowAllParametersOfDeclarationOnNextLine: false

That works. 😄

So it's still just the const ending up on its own line when it's the only thing over the 80 column mark that is the problem:

AlignAfterOpenBracket: BlockIndent
// ----------------------------------------------------------- Column Width 80 >|<

// Good: Fits on one line
bool ClassName::method(const bool &paramter1, const bool &parameter2) const {
  return true;
}

// Bad: Unexpected const on its own!
bool ClassName::longMethodName(const bool &paramter1, const bool &parameter2)
    const {
  return true;
}

// Good: parameters on the next line,
// closing parenthesis and const on the following line.
bool ClassName::veryVeryLongMethodName(
    const bool &paramter1, const bool &parameter2
) const {
  return true;
}

gedare added a commit to gedare/llvm-project that referenced this issue Jul 10, 2024
Fixes llvm#55731
Fixes llvm#73584

The reported formatting problems were related to ignoring deep nesting
of "simple" functions (causing llvm#54808) and to allowing the trailing
annotation to become separated from the closing parens, which allowed a
break to occur between the closing parens and the trailing annotation.
The fix for the nesting of "simple" functions is to detect them more
carefully. "Simple" was defined in a comment as being a single
non-expression argument. I tried to stay as close to the original intent
of the implementation while fixing the various bad formatting reports.

In the process of fixing these bugs, some latent bugs were discovered
related to how JavaScript Template Strings are handled. Those are also
fixed here.
gedare added a commit to gedare/llvm-project that referenced this issue Jul 23, 2024
Fixes llvm#55731
Fixes llvm#73584

The reported formatting problems were related to ignoring deep nesting
of "simple" functions (causing llvm#54808) and to allowing the trailing
annotation to become separated from the closing parens, which allowed a
break to occur between the closing parens and the trailing annotation.
The fix for the nesting of "simple" functions is to detect them more
carefully. "Simple" was defined in a comment as being a single
non-expression argument. I tried to stay as close to the original intent
of the implementation while fixing the various bad formatting reports.

In the process of fixing these bugs, some latent bugs were discovered
related to how JavaScript Template Strings are handled. Those are also
fixed here.
@owenca owenca closed this as completed in ccae7b4 Jul 25, 2024
yuxuanchen1997 pushed a commit that referenced this issue Jul 25, 2024
Summary:
Fixes #55731

The reported formatting problems were related to ignoring deep nesting
of "simple" functions (causing #54808) and to allowing the trailing
annotation to become separated from the closing parens, which allowed a
break to occur between the closing parens and the trailing annotation.
The fix for the nesting of "simple" functions is to detect them more
carefully. "Simple" was defined in a comment as being a single
non-expression argument. I tried to stay as close to the original intent
of the implementation while fixing the various bad formatting reports.

In the process of fixing these bugs, some latent bugs were discovered
related to how JavaScript Template Strings are handled. Those are also
fixed here.

---------

Co-authored-by: Owen Pan <[email protected]>

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250557
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants