This is an archive of the discontinued LLVM Phabricator instance.

Paths

Table of Contentst

4/10
.editorconfig

Differential D135789

Add .editorconfig file
AcceptedPublic

Authored by beanz on Oct 12 2022, 9:43 AM.

Download Raw Diff

Details

Reviewers

phosek
compnerd
aaron.ballman
MaskRay
rnk

Summary

EditorConfig is a pretty simple format that is supported by a bunch of
common code editors (including Visual Studio). This file allows setting
some of the editor settings to match LLVM coding style guidelines which
will hopefully make it so contributors using those editors write code
closer to LLVM style without fighting their editors.

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

beanz created this revision.Oct 12 2022, 9:43 AM

Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2022, 9:43 AM

Herald added a subscriber: StephenFan. · View Herald Transcript

beanz requested review of this revision.Oct 12 2022, 9:43 AM

Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2022, 9:43 AM

Just to add some extra context. Adding this to the root of the repo will get these settings set for Visual Studio, so anyone contributing to any LLVM sub-project using Visual Studio will have their editor obey these settings unless they specifically disable the default-on option to follow the project's coding style.

Harbormaster completed remote builds in B191761: Diff 467177.Oct 12 2022, 10:51 AM

mgorny added a subscriber: mgorny.Oct 12 2022, 11:58 AM

mgorny added inline comments.

.editorconfig
10	Does it have some key not to add empty trailing lines? ;-)

beanz added inline comments.Oct 12 2022, 2:12 PM

.editorconfig
10	The `insert_final_newline` adds that line, which is generally what we want because strict C states that all files should end with a newline. Both GCC and Clang have warnings for that.

This seems useful as it does apply to a lot of editors. If it were Visual Studio alone I would be unsure whether this passed the bar :)

A lot of .py files use 4 space indentation, will indent_size = 2 be problematic?
For CMake files I don't know a commonly used indentation. Will indent_size = 2 be problematic?
Consider whether indent_size = 2 should only be applied to C/C++ files.

.editorconfig
10	Remove the trailing blank line.
10	Do we need `end_of_line = lf`?

mgorny added inline comments.Oct 12 2022, 9:35 PM

.editorconfig
10	The `insert_final_newline` adds that line, which is generally what we want because strict C states that all files should end with a newline. Both GCC and Clang have warnings for that. But there are two newlines at the end of the file now.

Removing extra newline :(

Harbormaster completed remote builds in B191958: Diff 467465.Oct 13 2022, 8:38 AM

Do we want to add .editorconfig to .gitignore so users are free to modify their local copy for additional options supported by their editor without accidentally committing the changes for everyone else? Or is that a bad idea?

.editorconfig
9	Do these settings apply to the whole file when it is saved, or only to the modifications in the file as they're being made? I ask because of `trim_trailing_whitespace` specifically -- I have a text editor that does that automatically already when saving a file and I have to configure it to not do that because of how often it ends up making unrelated changes in a file when saving; especially our older test cases in Clang.

beanz added inline comments.Oct 13 2022, 12:34 PM

.editorconfig
9	Do these settings apply to the whole file when it is saved, or only to the modifications in the file as they're being made? I ask because of `trim_trailing_whitespace` specifically -- I have a text editor that does that automatically already when saving a file and I have to configure it to not do that because of how often it ends up making unrelated changes in a file when saving; especially our older test cases in Clang. That's a really good point. The documentation on https://editorconfig.org doesn't specify, so it might be editor dependent, but probably impacts the whole file.
10	Do we need `end_of_line = lf`? We could. I think at some point I'd like to get LLVM & Clang building and passing tests with autocrlf enabled at which point that would become a potential problem.

Adding end_of_line, removing trim_trailing_whitespace

aaron.ballman added inline comments.Oct 13 2022, 12:45 PM

.editorconfig
9	That's a really good point. The documentation on https://editorconfig.org doesn't specify, so it might be editor dependent, but probably impacts the whole file. If these impact the whole file, that's... troublesome for the trailing whitespace and the indentation fields because both of those will likely lead to unrelated edits in files. Perhaps we should see what supported editors do currently? If they all mostly agree and don't modify the whole file, maybe we can get away with it? We could. I think at some point I'd like to get LLVM & Clang building and passing tests with autocrlf enabled at which point that would become a potential problem. Can we opt specific files out of autocrlf (Clang has a handful of test cases that are line ending sensitive)?

In D135789#3854248, @MaskRay wrote:

This seems useful as it does apply to a lot of editors. If it were Visual Studio alone I would be unsure whether this passed the bar :)

A lot of .py files use 4 space indentation, will indent_size = 2 be problematic?

We have a mix here. Some python files are 2 and some are 4. I think having the editor formalize on 2 really just means that we set the same default as for C/C++ source, and I think that's probably good.

For CMake files I don't know a commonly used indentation. Will indent_size = 2 be problematic?

For CMake we definitely prefer 2-space indent, if there is CMake code differing from that it probably has more to do with unintentional editor wonkiness than anything else.

Consider whether indent_size = 2 should only be applied to C/C++ files.

beanz added inline comments.Oct 13 2022, 12:52 PM

.editorconfig
9	Can we opt specific files out of autocrlf (Clang has a handful of test cases that are line ending sensitive)? We can, and we already do in some cases. We have a couple `.gitattributes` files around the repo that specify specific line ending behaviors for certain files. Clang has a lot of tests that are line ending sensitive, and my to-do list includes reviewing those to see which ones _should_ be line ending sensitive and if we can address them. Last year (or maybe it was earlier this year) I made a bunch of tests not line-ending sensitive by making column and file-offset values fuzzy-matched.

Harbormaster completed remote builds in B192034: Diff 467574.Oct 13 2022, 1:53 PM

LGTM. I think this is good but this patch needs more opinions and ideally some manual verification.

This revision is now accepted and ready to land.Nov 1 2022, 7:13 PM

Revision Contents

Path

Size

.editorconfig

8 lines

Diff 467574

.editorconfig

This file was added.

				root = true

				[*]
				insert_final_newline = true
				indent_style = space
				indent_size = 2
				charset = utf-8
				end_of_line = lf
				mgornyUnsubmitted Not Done Reply Inline Actions Does it have some key not to add empty trailing lines? ;-) mgorny: Does it have some key not to add empty trailing lines? ;-)
				beanzAuthorUnsubmitted Done Reply Inline Actions The `insert_final_newline` adds that line, which is generally what we want because strict C states that all files should end with a newline. Both GCC and Clang have warnings for that. beanz: The `insert_final_newline` adds that line, which is generally what we want because strict C…
				mgornyUnsubmitted Not Done Reply Inline Actions The `insert_final_newline` adds that line, which is generally what we want because strict C states that all files should end with a newline. Both GCC and Clang have warnings for that. But there are two newlines at the end of the file now. mgorny: > The `insert_final_newline` adds that line, which is generally what we want because strict C…
				MaskRayUnsubmitted Not Done Reply Inline Actions Remove the trailing blank line. MaskRay: Remove the trailing blank line.
				MaskRayUnsubmitted Not Done Reply Inline Actions Do we need `end_of_line = lf`? MaskRay: Do we need `end_of_line = lf`?
				beanzAuthorUnsubmitted Done Reply Inline Actions Do we need `end_of_line = lf`? We could. I think at some point I'd like to get LLVM & Clang building and passing tests with autocrlf enabled at which point that would become a potential problem. beanz: > Do we need `end_of_line = lf`? We could. I think at some point I'd like to get LLVM & Clang…
				aaron.ballmanUnsubmitted Not Done Reply Inline Actions Do these settings apply to the whole file when it is saved, or only to the modifications in the file as they're being made? I ask because of `trim_trailing_whitespace` specifically -- I have a text editor that does that automatically already when saving a file and I have to configure it to not do that because of how often it ends up making unrelated changes in a file when saving; especially our older test cases in Clang. aaron.ballman: Do these settings apply to the whole file when it is saved, or only to the modifications in the…
				beanzAuthorUnsubmitted Done Reply Inline Actions Do these settings apply to the whole file when it is saved, or only to the modifications in the file as they're being made? I ask because of `trim_trailing_whitespace` specifically -- I have a text editor that does that automatically already when saving a file and I have to configure it to not do that because of how often it ends up making unrelated changes in a file when saving; especially our older test cases in Clang. That's a really good point. The documentation on https://editorconfig.org doesn't specify, so it might be editor dependent, but probably impacts the whole file. beanz: > Do these settings apply to the whole file when it is saved, or only to the modifications in…
				aaron.ballmanUnsubmitted Not Done Reply Inline Actions That's a really good point. The documentation on https://editorconfig.org doesn't specify, so it might be editor dependent, but probably impacts the whole file. If these impact the whole file, that's... troublesome for the trailing whitespace and the indentation fields because both of those will likely lead to unrelated edits in files. Perhaps we should see what supported editors do currently? If they all mostly agree and don't modify the whole file, maybe we can get away with it? We could. I think at some point I'd like to get LLVM & Clang building and passing tests with autocrlf enabled at which point that would become a potential problem. Can we opt specific files out of autocrlf (Clang has a handful of test cases that are line ending sensitive)? aaron.ballman: > That's a really good point. The documentation on https://editorconfig.org doesn't specify, so…
				beanzAuthorUnsubmitted Done Reply Inline Actions Can we opt specific files out of autocrlf (Clang has a handful of test cases that are line ending sensitive)? We can, and we already do in some cases. We have a couple `.gitattributes` files around the repo that specify specific line ending behaviors for certain files. Clang has a lot of tests that are line ending sensitive, and my to-do list includes reviewing those to see which ones _should_ be line ending sensitive and if we can address them. Last year (or maybe it was earlier this year) I made a bunch of tests not line-ending sensitive by making column and file-offset values fuzzy-matched. beanz: > Can we opt specific files out of autocrlf (Clang has a handful of test cases that are line…

This is an archive of the discontinued LLVM Phabricator instance.

Add .editorconfig fileAcceptedPublic

Details

Diff Detail

Event Timeline

Revision Contents

Diff 467574

.editorconfig

Add .editorconfig file
AcceptedPublic