llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.66k stars 11.85k forks source link

clang-format single-line enums AllowShortEnumsOnASingleLine and ColumnLimit = 0 #60475

Open vesterij opened 1 year ago

vesterij commented 1 year ago

I am trying to have short single line enum unchanged when ColumnLimit = 0

clang-format version 15.0.1

Config

Language:        Cpp
AllowShortEnumsOnASingleLine: true
ColumnLimit:     0

Code

// Can this be single line?
enum class IsItWorking { Yes, No, Maybe };

Result

// Can this be single line?
enum class IsItWorking { Yes,
                         No,
                         Maybe };

If I set ColumnLimit to 120, enum will remain one-liner but then clang-format wont respect existing line breaks which I do not want.

I debugged the code and with this example we do not even reach the line where AllowShortEnumsOnASingleLine is used.

Is this a bug or is there some other configuration that conflicts keeping enum as a one-liner?

Backl1ght commented 1 year ago

The line breaks is created because Indenter->mustBreak(State) in the codeblock below for token No and Maybe is true.

https://github.com/llvm/llvm-project/blob/ed740e741ec22f9aaea09bfc0b87d0801a7c492f/clang/lib/Format/UnwrappedLineFormatter.cpp#L1068-L1075

This is a bug I think.

llvmbot commented 1 year ago

@llvm/issue-subscribers-clang-format

Backl1ght commented 1 year ago

For token No and Maybe, Indenter->mustBreak(State) return true in the following branch.

https://github.com/llvm/llvm-project/blob/5e01234df81885fa882c58e062ca0cb87ac4849d/clang/lib/Format/ContinuationIndenter.cpp#L380-L395

vesterij commented 1 year ago

For temporary fix I just added this in the beginning of ContinuationIndenter::mustBreak and using custom version for now

  // AllowShortEnumsOnASingleLine Fix
  if (Style.AllowShortEnumsOnASingleLine &&
      State.Line->First->is(tok::kw_enum) && 
      State.StartOfLineLevel == 0) {
    return false;
  }

Edit: Checking if original statement was multi-line has to be done in a different way HasUnescapedNewline was not the way. Edit2: Replaced condition with "State.StartOfLineLevel == 0" which is a hack. And also didn't work in all places.

There are also larger multi-line enums in the code base and I do not want to change them to single line (and I would like to have line break after multi-line enum brace, which means that BraceWrapping -> AfrerEnum is also set to true), This seems to work correctly in all my use cases.

So both cases are more like this:

// Can this be single line?
enum class IsItWorking { Yes, No, Maybe };

// Can this remain multi-line?
enum class LongEnum
{
    Error = 0, // Not set
    First = 1, // this is first
    Second = 2, // this is second
    Third = 3, // comment
    Last = 4 // Last
};

// Reformat this, please.
enum class Weekday { 
    Monday, Tuesday, Wednesday,
    Thursday,
    Friday, Saturday,
    Sunday
};  

Current result:

// Can this be single line?
enum class IsItWorking { Yes, No, Maybe };

// Can this remain multi-line?
enum class LongEnum
{
    Error = 0,  // Not set
    First = 1,  // this is first
    Second = 2,  // this is second
    Third = 3,  // comment
    Last = 4  // Last
};

// Reformat this, please.
enum class Weekday {
    Monday,
    Tuesday,
    Wednesday,
    Thursday,
    Friday,
    Saturday,
    Sunday
};

Weekday brace doesn't format correctly. I'll try again later

vesterij commented 1 year ago

Fixed my temporary fix. Ignore last comment.

There was no simple way to check if original statement had line breaks so I added helper function to AnnotatedLine

  // Helper to check if original statement had line breaks
  bool wasMultiline() const {
      // Skip first because of previous line break
      const FormatToken* token = First->Next; 
      while(token) {
          if (token->HasUnescapedNewline) {
              return true;
          }
          token = token->Next;
      }
      return false;
  }

And ContinuationIndenter::mustBreak

  // AllowShortEnumsOnASingleLine Fix
  if (Style.AllowShortEnumsOnASingleLine &&
      State.Line->First->is(tok::kw_enum) &&
      !State.Line->wasMultiline()) {
      return false;
  }

Now all my cases work as I expect.

This is not right way to fix this. I think AllowShortEnumsOnASingleLine should work similar way as AllowShortFunctionsOnASingleLine