Experiment with clang-format. #356
Conversation
|
Thanks for making the formating uniform, @vasil-pashov. Formatting is useful, yet it can create obfuscation. We might want to ignore commits in the blame view for GitHub and have people adapt their local setup with What do you think? |
|
@jjerphan adding a I didn't get this part, however:
|
vasil-pashov
force-pushed
the
clang-format-wip
branch
from
52f43c8
to
82b50a6
Compare
Oh, I wanted to say that the setup is automatic on GitHub but that it isn't locally. Locally, one needs to adapt the configuration with: git config blame.ignoreRevsFile .git-blame-ignore-revs |
The style is based on LLVM. With the following differences: * AccessModifierOffset: -2 -> -4 * AlignAfterOpenBracket: Align -> DontAlign * AllowAllArgumentsOnNextLine: true -> false * AllowAllParametersOfDeclarationOnNextLine: true -> false * AllowShortEnumsOnASingleLine: true -> false * AllowShortFunctionsOnASingleLine: All -> None * AlwaysBreakTemplateDeclarations: MultiLine -> Yes * BinPackArguments: true -> false * BinPackParameters: true -> false * AllowAllConstructorInitializersOnNextLine: true -> false * IndentWidth: 2 -> 4 * ObjCBlockIndentWidth: 2 -> 4 * ReflowComments true -> false * SpaceAfterTemplateKeyword: true -> false * SortIncludes: true -> false // Sorting the includes makes the build to fail * ColumnLimit: 80 -> 120 * PackConstructorInitializers: BinPack -> Never * PointerAlignment: Right -> Left * AfterControlStatement: false -> MultiLine * AfterFunction: false -> true * BreakBeforeBraces: Attach -> Custom
vasil-pashov
force-pushed
the
clang-format-wip
branch
from
82b50a6
to
94c8de7
Compare
| AsyncStore(std::shared_ptr<storage::Library> library, const arcticdb::proto::encoding::VariantCodec& codec) | ||
| : library_(std::move(library)), | ||
| codec_(new arcticdb::proto::encoding::VariantCodec{codec}) | ||
| { |
There was a problem hiding this comment.
There are basically 3 things we can do:
void function(int param1,
int param2,
int param3,
int param4) {
//function body
}
Pros: most of the function will stay the same
Cons: When wrapping happens the last param and the function body are indented the same and it's kind of hard to tell where params end and where the body starts
void function(int param1,
int param2,
int param3,
int param4)
{
//function body
}
Pros: Clear separation between the end of the function params and the start of the body
Cons: Changes each function
void function(int param1,
int param2,
int param3,
int param4) {
//function body
}
Pros: Provides some separation (not always) between the body and the end of the params.
Cons: (Personal opinion) looks weird. Will change lots of functions
| } | ||
|
|
||
| folly::Future<entity::VariantKey> write( | ||
| stream::KeyType key_type, | ||
| folly::Future<entity::VariantKey> write(stream::KeyType key_type, |
There was a problem hiding this comment.
I prefer: once wrapping starts, the first argument should be wrapped as well.
There was a problem hiding this comment.
Not possible with clang format as of today.
| std::move(segment), | ||
| library_, | ||
| codec_, | ||
| size_t(0)}) |
There was a problem hiding this comment.
I really like the original two-line wrapping for this call. When I read that, my brain is able to parse the structure quickly and I can then focus on the important bits.
The long wrapping, on the other hand, robs the focus and I have to triple read to make the connection between submit_cpu_task and via below.
There was a problem hiding this comment.
There are rare occasions where we don't want to enforce wrapping. If we don't enforce it it will dissolve into anarchy. On the other hand I feel like having multiple things on the same line is confusing and easy to misread.
| timestamp creation_ts, | ||
| IndexValue start_index, | ||
| IndexValue end_index, | ||
| SegmentInMemory&& segment) override |
There was a problem hiding this comment.
When Egpyption brackets are used, the argument list needs extra indentation to offset it from the function body.
| folly::Future<VariantKey> copy(KeyType key_type, const StreamId& stream_id, VersionId version_id, const VariantKey& source_key) override { | ||
| return async::submit_io_task(CopyCompressedTask<ClockType>{source_key, key_type, stream_id, version_id, library_}); | ||
| folly::Future<VariantKey> | ||
| copy(KeyType key_type, const StreamId& stream_id, VersionId version_id, const VariantKey& source_key) override |
There was a problem hiding this comment.
Is it possible to prefer wrapping the arguments first instead of the type?
There was a problem hiding this comment.
I don't think there's a direct option to this. But you can assign some penalty value, so it prefers putting the type on the same line.
The style is based on LLVM. With the following differences:
fail