-
Notifications
You must be signed in to change notification settings - Fork 10
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
Initial clang-format file #39
Conversation
**Reason:** Some editors interpret CRLF (`\r\n`) as two linebreaks. LF (`\n`) | ||
is always interpreted correctly. | ||
|
||
**Enforced by:** | ||
* `.clang-format`: `DeriveLineEnding: false` | ||
* `.clang-format`: `UseCRLF: false` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually never had a problem with line endings, because git handles this automatically. So I wonder if we need to set anything here. I am afraid that it could mess up when formatting on Windows machines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line endings depends on your git configuration. IMO if we enforce it we will avoid issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far this does not seem to be an issue on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this turned out to be an issue for Windows now. Given a file with sections where clang-format is disabled:
...
// clang-format off
...
// clang-format on
...
Git by default will checkout with CRLF on Windows as it should. When you clang-format now, all lines are switched to LF, EXCEPT the lines where clang-format is disabled. This results in a file with mixed line endings and some IDEs rightfully complain now. Formatting the entire file using LF solves the IDE problem but then shows the entire file as changed in git.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am now wondering if we shouldn't enforce the correct line ending through git rather than clang-format. This should prevent the issues described by Bernhard.
**Reason:** This gives the code a nice vertical structure and optically | ||
separates different scopes from another. The K&R indentation style | ||
unnecessarily increases code density, making it harder to follow its | ||
structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the given **Reason**
is correct, but one could elaborate on why it is better at optically separat[ing] different scopes
: The K&R example you show would indeed not be improved much by Allman, but K&R breaks down when the block opening statement needs to be broken to the next line:
if ( something(somethingElse) < moreElse
&& something(somethingElseElse) > 0) {
doSomething();
}
The Java Variant also breaks down for constructors, which happens more often:
ClassName::ClassName(Type a, ....)
: m_a(a), /*more things here so this cannot be inline*/.. {
m_b = f(a);
}
Granted, this could be worked around by having ContinuationIndentWidth > IndentWidth
and ConstructorInitializerIndentWidth > IndentWidth
, respectively, but this is not currently proposed, and would also feel strange to me.
Not sure if the
struct Name {
in your example is the Java variant, or either, but this gets into the same trouble when a list of base classes is added.
|
||
### Align consecutive assignments, bitfields and macros. | ||
|
||
**Reason:** It is visually pleasing and there is no reason against it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started out aligning everything as well, but I ran into cases where it was not visually pleasing:
for () {
...
someLongVariableName += 1;
result = thisIsAFunctionCall(withSomeArguments, andAnother);
i += offset;
}
It happens ;) So I started to be more defensive here. But find out yourself ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not very emotional on this one, as it does not happen often, but to me such alignments feel very bad, or to mirror the reasoning text visually ugly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alignments are bad because they are not invariant under refactoring, i.e. if you change the length of the longest variable name (or add a new longer one) all the others need to change, too.
In addition tom this, I think, prescribing alignment is only an option if we could configure a pre-commit hook running a fixer. I would not want to spent half my time coding aligning things, or fighting with my editor if it is configured to do this automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alignments are bad because they are not invariant under refactoring, i.e. if you change the length of the longest variable name (or add a new longer one) all the others need to change, too.
I totally agree. Alignment leads to unnecessarily large diffs. Especially with short line length, parameters to functions, templates, etc. are often on multiple lines. If you rename a function, template, etc., then all the parameters in all invocations have to be realigned as well.
This is the reason why the proposed alpaka style had all alignments disabled:
I am ok with the options below but I dislike the AlignAfterOpenBracket
option.
I would prefer if the above example would look like:
auto some_long_function(
argument1,
argument2);
especially when you have multiple methods in a header, the aligned style looks very noisy while the indentation makes this look clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried the suggested .clang-format file on LLAMA. I am not sure if I like this one:
constexpr std::size_t hardwareThreads = 2; // relevant for OpenMP2Threads
using Distribution = common::ThreadsElemsDistribution<Acc, BLOCK_SIZE, hardwareThreads>;
constexpr std::size_t elemCount = Distribution::elemCount;
constexpr std::size_t threadCount = Distribution::threadCount;
constexpr FP ts = 0.0001;
This one turned out nice:
constexpr auto DEFAULT_IMG_X = 4096; /// width of the default image if no png is loaded
constexpr auto DEFAULT_IMG_Y = 4096; /// height of the default image if no png is loaded
constexpr auto KERNEL_SIZE = 8; /// radius of the blur kernel, the diameter is this times two plus one
constexpr auto CHUNK_SIZE = 512; /// size of each chunk to be processed per alpaka kernel
constexpr auto ELEMS_PER_BLOCK = 16; /// number of elements per direction(!) every block should process
I do not like this one:
int x = 0;
int y = 0;
int n = 3;
unsigned char* data = stbi_load(argv[1], &x, &y, &n, 0);
I feel like I am weakly against alignment. Also the reasons given by others, like larger diffs, make me feel aligning is not a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See vote (#39 (comment))
|
||
### `goto` labels are not indented. | ||
|
||
***DO NOT USE `goto`!*** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would turn that into: Use goto sparingly. Or just reference the C++ core guidelines: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-goto
Since code style based on own opinions it is hard to find a con-sens. To be clear, after we have a common code style the style file will be pushed to all our projects, including PIConGPU, cupla, alpaka, ... Rules
Voting:
Example
If you thing someone should attend this code style finding process please provide the link of this PR to him/here. CC-ing: @ComputationalRadiationPhysics/picongpu-developers @ComputationalRadiationPhysics/alpaka-developers |
**Reason:** This makes the documentation easier to read. | ||
|
||
**Enforced by:** | ||
* `.clang-format`: `AlignTrailingComments: true` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a bad experience with this option together with ReflowComments
and short line lengths. Hopefully this has been fixed.
I like the format proposed by @psychocoderHPC. In addition I suggest that anyone who starts a vote also needs to supply an accompanying change to |
Please also note the separate vote in #40. |
Since the interval ended for several votes this weekend, I prepared a new .clang-format file and ran it on the current |
There are no more open votes. If there won't be any new votes until Friday I will close this PR - all options not voted for will have their current values then. |
I opened another vote, which apparently went unnoticed: about tabs. |
There are no more open votes. If there won't be any new votes until Friday I will close this PR - all options not voted for will have their current values then. |
This is a first proposal for the clang-format file along with a set of rules written in Markdown.
[update psychocoderHPC]
Please read #39 (comment) before you start to comment on this PR.