[clang-format][NFC] Be more careful about the layout of FormatToken.

[riccibruno]

Summary

The underlying ABI forces FormatToken to have a lot of padding. Currently (on x86-64 linux) sizeof(FormatToken) == 288. After this patch sizeof(FormatToken) == 232.

No functional changes.

[MyDeveloperDay]

clang/lib/Format/FormatToken.h

177

educate me, why

unsigned IsFirst : 1;

here and not

bool IsFirst : 1;

is that equivalent? (I'm literally not sure myself), I wrote a little test just to remind myself how this stuff works.

#include <iostream>

class Foo
{
public:
    Foo()
      : A(true)
      , B(false)
      , C(true)
    {
    }
    bool A : 1;
    bool B : 1;
    bool C : 1;
};

class Bar
{
public:
    Bar()
      : A(true)
      , B(false)
      , C(true)
    {
    }
    unsigned A : 1;
    unsigned B : 1;
    unsigned C : 1;
};

class Fuz
{
public:
    Fuz()
      : A(true)
      , B(false)
      , C(true)
    {
    }
    bool A;
    bool B;
    bool C;
};

class Baz
{
public:
    Baz()
      : A(true)
      , B(false)
      , C(true)
    {
    }
    unsigned A;
    unsigned B;
    unsigned C;
};

int
main(int argc, char *argv[])
{
    std::cerr << "Foo " << sizeof(Foo) << "\n";
    std::cerr << "Bar " <<sizeof(Bar) << "\n";
    std::cerr << "Fuz " <<sizeof(Fuz) << "\n";
    std::cerr << "Baz " <<sizeof(Baz) << "\n";

    return 0;
}

When run gives the following:

Foo 1
Bar 4
Fuz 3
Baz 12

So I guess my question is could there be more space savings if using bool IsFirst:1 (and the others), I'd also think that would help clarity a little (or did I misunderstand?)

[MyDeveloperDay]

Thank you for the patch, I'm generally in agreement but I think we should let some others comment

clang/lib/Format/ContinuationIndenter.cpp

652

I think this is better in that its now easier perhaps to see when the block kind gets checked:

I wonder if it would read even better as if we added is(BraceBlockKind) isNot(BraceBlockKind)

e.g.

Previous.is(BK_BraceInit)

clang/lib/Format/FormatToken.h

151

I much prefer putting the initialization here, I think it makes it MUCH clearer

[riccibruno]

clang/lib/Format/ContinuationIndenter.cpp

652

Yes I think I agree with you; adding is and isNot would be an improvement. I will still leave getBlockKind if someone wants to switch on it.

clang/lib/Format/FormatToken.h

151

It is necessary anyway since a bit-field cannot have a default member initializer pre-C++20.

177

It has to do with the ABI since as per [class.bit]p1:

[...] Allocation of bit-fields within a class object is implementation-defined. Alignment of bit fields is implementation-defined. Bit-fields are packed into some addressable allocation unit. [Note: Bit-fields straddle allocation units on some machines and not on others. Bit-fields are assigned right-to-left on some machines, left-to-right on others. — end note ]

Now the two relevant ABIs are the Itanium ABI (https://github.com/itanium-cxx-abi/cxx-abi/blob/master/abi-layout.html for the details) and the MS ABI (say on x86-64). Happily both are supported by clang so we can use -fdump-record-layouts and compare them.

Consider S0 in https://godbolt.org/z/orYv5j (itanium on the left/MS on the right):

Both ABIs will not let a bit-field cross a boundary. Therefore c0 and c1 will use 10 bits. If the type had been short instead of char then only 9 bits would have been used. The size of S0 would still have been 2 in both cases.

Consider now S1. The MS ABI, unlike the Itanium ABI, will not put bit-fields together if their types have a different size. Therefore sizeof(S1) is 2 under the Itanium ABI and 4 under the MS ABI.

Using an unsigned systematically avoids having a boundary every 8 bits, and avoids the issue with the MS ABI.

[riccibruno]

Use is and isNot.

[MyDeveloperDay]

a new minor Nits but looks good.

clang/lib/Format/ContinuationIndenter.cpp

1487–1488

Current.is(PPK_OnePerLine)

1488–1489

Current.is(PPK_Inconclusive)

[riccibruno]

Remove a few missed getPackingKind.

[MyDeveloperDay]

Thank you for the patch, this LGTM, I think this kind of change should help reduce memory usage and I feel improves the readability.

[riccibruno]

In D84306#2177985, @MyDeveloperDay wrote:

Thank you for the patch, this LGTM, I think this kind of change should help reduce memory usage and I feel improves the readability.

Thanks for your review!

[klimek]

FWIW, finding this due to seeing the added complexity. Do you have data on what the difference is on clang-format for either overall memory use or performance? Thanks!

[MyDeveloperDay]

Which bit do you find more complex? adding something to the FormatToken or the use of the is() calls?

[klimek]

In D84306#2294952, @MyDeveloperDay wrote:

Which bit do you find more complex? adding something to the FormatToken or the use of the is() calls?

I think mainly that
a) the language doesn't support bitfields well yet, as evidenced by having to pull the default values into the constructor, where it's harder to maintain imo (but that will change)
b) we'll need to continuously take care that the bitfields stay bundled

The setters / getters are generally fine, I think; I believe the decision to treat this as a struct from the very beginning of the project was bad, but refactoring this into something more cohesive seems like a daunting task.

To be clear, I don't think it's a lot of complexity, I was mainly wondering whether we have some measurement what it gets us.