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.
Paths
| Differential D84306
[clang-format][NFC] Be more careful about the layout of FormatToken. ClosedPublic Authored by riccibruno on Jul 22 2020, 4:04 AM.
Details 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.
Diff Detail
Event Timeline
Comment Actions Thank you for the patch, I'm generally in agreement but I think we should let some others comment
riccibruno added inline comments.
Comment Actions Thank you for the patch, this LGTM, I think this kind of change should help reduce memory usage and I feel improves the readability. This revision is now accepted and ready to land.Jul 28 2020, 2:01 AM Comment Actions
Thanks for your review! Closed by commit rGf5acd11d2c0e: [clang-format][NFC] Be more careful about the layout of FormatToken. (authored by riccibruno). · Explain WhyJul 28 2020, 2:36 AM This revision was automatically updated to reflect the committed changes. Comment Actions 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! Comment Actions Which bit do you find more complex? adding something to the FormatToken or the use of the is() calls? Comment Actions
I think mainly that 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.
Revision Contents
Diff 281155 clang/lib/Format/ContinuationIndenter.cpp
clang/lib/Format/Format.cpp
clang/lib/Format/FormatToken.h
clang/lib/Format/FormatToken.cpp
clang/lib/Format/TokenAnnotator.cpp
clang/lib/Format/UnwrappedLineFormatter.cpp
clang/lib/Format/UnwrappedLineParser.cpp
clang/lib/Format/WhitespaceManager.cpp
|
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)