Issue 29537634: Noissue - clang-format configuration file for C++
|
Created:
Sept. 6, 2017, 1:44 p.m. by Eric Modified:
Nov. 24, 2017, 11:30 a.m. CC:
tlucas Visibility:
Public. |
DescriptionNoissue - clang-format configuration file for C++
Patch Set 1 #
Total comments: 7
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : new version #
MessagesTotal messages: 21
This is for codingtools. It's a configuration file for clang-format to eyeo coding standards. I first worked on it last summer (a few versions back for clang-format); I haven't audited it to see if there are new keywords we should be using. Felix has asked that I get this into the repository. We would like to integrate this with C++ build systems. This is an initial stem toward that goal.
we should file an issue for this as we'll need to also update the dependencies in the module.
On 2017/09/06 14:21:53, hub wrote: > we should file an issue for this as we'll need to also update the dependencies > in the module. Not yet - as the first step we'll just add instructions on how to run this manually to the README.md. But an issue still makes sense :) Also: Added Sergei and Ollie as reviewers.
Current this clang format cause this kind of changes:
- scheduler([this, fileName, data, callback]
- {
+ scheduler([this, fileName, data, callback] {
While I do agree with clang-format, this is not the style we have been using.
https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl...
File c++/clang-format/clang-format-eyeo (right):
https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl...
c++/clang-format/clang-format-eyeo:18: ColumnLimit: 0
Should we have a column limit like 80 ? it is loosely applied, but 0 make some
line being unwrapped.
https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl... File c++/clang-format/clang-format-eyeo (right): https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl... c++/clang-format/clang-format-eyeo:28: IndentCaseLabels: false This should be "true" False turns ``` switch () { case 1: ``` to ``` switch () { case 1: ``` I have seen the later style though (in a few instances), so we should just settle on one and set this options accordingly.
On 2017/09/06 14:41:23, Felix Dahlke wrote:
> On 2017/09/06 14:21:53, hub wrote:
> > we should file an issue for this as we'll need to also update the
dependencies
> > in the module.
>
> Not yet - as the first step we'll just add instructions on how to run this
> manually to the README.md. But an issue still makes sense :)
I'm not sure whether everyone is aware how it works and what it can do, so I
would like to add it here. It's actually not a style checker (like pep8, for
instance), it's a code formatter, it means that it does not tell what rule is
violated and where, instead it outputs a new formatted version of an input file.
In addition it can produce the replacements xml file with a list of replacements
(offsets, lenghts, values).
Therefore for the beginning I would propose to have some script which one can
run manually at any time and which makes the changes in our C++ files in place.
BTW, `find ./src/ -iname \*.cpp -exec clang-format -i {} \;` crashes... we need
something more advanced.
So, for instance, you put all your changes in the stage then run the formatter,
and see whether you agree with the proposed changes, afterwards you can either
revert the changes or apply them. Let's see how convenient it is.
There are also some things which cannot be configured, e.g. the space after
variable in a for-each loop
for (const auto& x: list)
vs
for (const auto& x : list).
I generally don't mind to have such changes, merely wanted you to be aware about
it.
https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl...
File c++/clang-format/clang-format-eyeo (right):
https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl...
c++/clang-format/clang-format-eyeo:3: AccessModifierOffset: -2
I think we should add
AlignAfterOpenBracket: DontAlign
https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl... File c++/clang-format/clang-format-eyeo (right): https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl... c++/clang-format/clang-format-eyeo:3: AccessModifierOffset: -2 On 2017/09/08 15:10:44, sergei wrote: > I think we should add > AlignAfterOpenBracket: DontAlign The Mozilla style guide uses examples where the value would be "Align". A dump of the Mozilla style within `clang-format` has the value "Align". I've added the "Align" value. We can change the Eyeo coding style, but that's out of scope for this change. https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl... c++/clang-format/clang-format-eyeo:18: ColumnLimit: 0 On 2017/09/06 14:42:42, hub wrote: > Should we have a column limit like 80 ? it is loosely applied, but 0 make some > line being unwrapped. I tried column limits before, and it wasn't practicable, not at the outset. A bunch of old code suddenly looked far worse. Lot's of lines that had been ~ 81-90 characters got orphaned second lines. Etc. Since this is an initial commit, it's more conservative not to enforce this one yet. I'm thinking we need a script wrapper around the use case of reformatting long lines. https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl... c++/clang-format/clang-format-eyeo:28: IndentCaseLabels: false I changed this one to conform to the Mozilla style guide. As I recall, this was to deal with legacy ablockplusie code or Visual Studio default (I forget which) that used the other style.
On 2017/09/06 14:42:43, hub wrote:
> Current this clang format cause this kind of changes:
>
> - scheduler([this, fileName, data, callback]
> - {
> + scheduler([this, fileName, data, callback] {
>
> While I do agree with clang-format, this is not the style we have been using.
Most of the C++ code I've seen has been Allman style. I would not, however, be
surprised that it wasn't entirely that way.
Once we get this committed, we can do some "Noissue - Format Changes Only"
change sets.
On 2017/09/08 15:10:45, sergei wrote: > I'm not sure whether everyone is aware how it works and what it can do, so I > would like to add it here. This change set has a very modest goal: to get an initial version of a format file into a repository. Anything past that is beyond the scope of the present change set. > Therefore for the beginning I would propose to have some script Any form of scripting is beyond the scope of this change set. Secondarily, any script immediately induces platform dependencies and that automation would affect the build system. Since we are in the middle of selecting a build system, scripting is also premature. For example, one of the merits of CMake is that it has an internal cross-platfrom shell for simple scripts that suffice for most utility purposes. If we go with CMake, we'd want to write any such script there. > I generally don't mind to have such changes, merely wanted you to be aware about > it. I'm well aware that any programmatic involvement with style is always incomplete.
On 2017/09/08 15:10:45, sergei wrote:
> Therefore for the beginning I would propose to have some script which one can
> run manually at any time and which makes the changes in our C++ files in
place.
> BTW, `find ./src/ -iname \*.cpp -exec clang-format -i {} \;` crashes... we
need
> something more advanced.
clang-format in that case will needs "-style=file" to be passed on the command
line and to have the format definition copied into .clang-format. It complicate
things.
https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl... File c++/clang-format/clang-format-eyeo (right): https://codereview.adblockplus.org/29537634/diff/29537635/c++/clang-format/cl... c++/clang-format/clang-format-eyeo:18: ColumnLimit: 0 On 2017/09/08 15:48:34, Eric wrote: > On 2017/09/06 14:42:42, hub wrote: > > Should we have a column limit like 80 ? it is loosely applied, but 0 make some > > line being unwrapped. > > I tried column limits before, and it wasn't practicable, not at the outset. A > bunch of old code suddenly looked far worse. Lot's of lines that had been ~ > 81-90 characters got orphaned second lines. Etc. > > Since this is an initial commit, it's more conservative not to enforce this one > yet. I'm thinking we need a script wrapper around the use case of reformatting > long lines. this actually unwrap manually wrapped lines. So maybe a different approach would be to ensure it doesn't unwrap. Not sure which option.
On 2017/09/06 14:42:42, hub wrote: > Should we have a column limit like 80 ? it is loosely applied, but 0 make some > line being unwrapped. P.S. Also on this issue, the eyeo style guide has an explicit exception against the default hard line length limit: "Lines can be longer than the limit, if limiting line length would hurt readability in a particular case."
On 2017/09/08 16:02:45, hub wrote: > this actually unwrap manually wrapped lines. So maybe a different approach would > be to ensure it doesn't unwrap. Not sure which option. Ah. I don't recall that's how it previously behaved. I'll look into it.
On 2017/09/08 16:02:45, hub wrote: > this actually unwrap manually wrapped lines. So maybe a different approach would > be to ensure it doesn't unwrap. Not sure which option. I couldn't replicate this behavior. I don't think the column limit rule is the problem. From the docs "A column limit of 0 means that there is no column limit. In this case, clang-format will respect the input’s line breaking decisions within statements unless they contradict other rules." Note that word "unless". Could you email me an example of code that's apparently being unwrapped?
On 2017/09/08 16:56:01, Eric wrote: > On 2017/09/08 16:02:45, hub wrote: > > this actually unwrap manually wrapped lines. So maybe a different approach > would > > be to ensure it doesn't unwrap. Not sure which option. > > I couldn't replicate this behavior. > > I don't think the column limit rule is the problem. From the docs "A column > limit of 0 means that there is no column limit. In this case, clang-format will > respect the input’s line breaking decisions within statements unless they > contradict other rules." Note that word "unless". Could you email me an example > of code that's apparently being unwrapped? in src/DefaultFileSystem.cpp #if _POSIX_C_SOURCE >= 200809L - result.lastModified = static_cast<int64_t>(nativeStat.st_mtim.tv_sec) * MSEC_IN_SEC - + static_cast<int64_t>(nativeStat.st_mtim.tv_nsec) / NSEC_IN_MSEC; + result.lastModified = static_cast<int64_t>(nativeStat.st_mtim.tv_sec) * MSEC_IN_SEC + static_cast<int64_t>(nativeStat.st_mtim.tv_nsec) / NSEC_IN_MSEC; #else
A couple of procedural comments: 1) Right now the folders in `codingtools` repository are called by the name of the tool rather than by language. To stay consistent with this naming scheme, it would make more sense to not have the `c++` folder here and place the README and `clang-format-eyeo` file into a folder called `clang-format-eyeo` or something like that. 2) See inline comment about README formatting below. Cheers, Vasily https://codereview.adblockplus.org/29537634/diff/29539824/c++/clang-format/RE... File c++/clang-format/README.md (right): https://codereview.adblockplus.org/29537634/diff/29539824/c++/clang-format/RE... c++/clang-format/README.md:4: A file containing style configuration for [`clang-format`](http://clang.llvm.org/docs/ClangFormat.html) We are generally trying to keep the lines under 80 characters in README files in this repo. It would be nice to reformat this README as well unless you think it would hurt readability. In cases of long links, one possible option is to convert them to footnote-like links like this: Lorem ipsum [dolor][1] sit amet... ...anim id est laborum. [1]: https://en.wiktionary.org/wiki/dolor
On 2017/09/08 17:37:01, hub wrote:
> in src/DefaultFileSystem.cpp
>
> #if _POSIX_C_SOURCE >= 200809L
> - result.lastModified = static_cast<int64_t>(nativeStat.st_mtim.tv_sec) *
> MSEC_IN_SEC
> - + static_cast<int64_t>(nativeStat.st_mtim.tv_nsec) /
> NSEC_IN_MSEC;
> + result.lastModified = static_cast<int64_t>(nativeStat.st_mtim.tv_sec) *
> MSEC_IN_SEC + static_cast<int64_t>(nativeStat.st_mtim.tv_nsec) / NSEC_IN_MSEC;
> #else
The best explanation of this is the accepted answer in the following Stack
Overflow question:
https://stackoverflow.com/questions/33656800/clang-format-line-breaks
The short answer is that clang-format doesn't really pay attention to whitespace
within the source file. I don't know how strictly true this is (I've not read
the code myself), but the above answer seems to indicate that it's at least
mostly true. I couldn't replicate this originally because I was using short line
fragments with arguments, rather than short line fragments within an expression,
so invoking a different set of rules.
The answer, though, does have a nice little hack in it that will address unusual
cases like this without much fuss. A '//' comment is a token that inherently
contains a newline, so clang-format can't get rid of it and must just deal with
it given its other rules. Adding such a comment, even empty, works fine. Here's
what the code snippet does with a comment. It still reformats, but only to put
the expression at a consistent column position.
- result.lastModified = static_cast<int64_t>(nativeStat.st_mtim.tv_sec) *
MSEC_IN_SEC //
- + static_cast<int64_t>(nativeStat.st_mtim.tv_nsec) /
NSEC_IN_MSEC;
+ result.lastModified = static_cast<int64_t>(nativeStat.st_mtim.tv_sec) *
MSEC_IN_SEC //
+ + static_cast<int64_t>(nativeStat.st_mtim.tv_nsec) /
NSEC_IN_MSEC;
There are also special comments to turn formatting off and on. That's overkill
for simple cases like the present one, but available for cases such as the
answer above contains.
I'm going to add this all to the README. It's not obvious, because it's not
explicit in the documentation, that clang-format is a complete reformatter and
not a format modifier for compliance with rules. The difference isn't large, but
it shows up in cases like your example.
Patch set 3 is a rewrite of the configuration file. The README is expanded. In tracking down Hubert's observation, I was seeing configurations options in the online documentation that weren't present in my original. As it turns out, clang-format has been changing its syntax at each release, and not in a backward compatible way. These changes are for the better, generally replacing simple boolean features with richer policy options. Nevertheless, they are changes. I had originally created that file by dumping the Mozilla configuration and then editing it. That was with Clang 3.x, where x is something I don't recall. What I had never done is to go back and get rid of the unchanged lines and replace them with a single "BasedOnStyle" line. That's what I've done with the rewrite. This should get us off most of the version treadmill. Clang 5 was released last week and changed the syntax of one of the options. I've defaulted to Clang 4, with the Clang 5 changes present but commented out. Clang 6 documentation is online, but dealing with that in any way is out of scope for the present. I've run clang-format on the libadblockplus code. It compiles and runs the unit tests successfully. Unlike with Clang 3, clang-format sorts the header includes, and I thought it prudent to make sure we didn't have any inclusion order issues. I did not change the inclusion order specification. If we don't like the default, we can change it in a future change set; I consider it out of scope for the present change set. https://codereview.adblockplus.org/29537634/diff/29539824/c++/clang-format/RE... File c++/clang-format/README.md (right): https://codereview.adblockplus.org/29537634/diff/29539824/c++/clang-format/RE... c++/clang-format/README.md:4: A file containing style configuration for [`clang-format`](http://clang.llvm.org/docs/ClangFormat.html) On 2017/09/11 10:44:33, Vasily Kuznetsov wrote: > We are generally trying to keep the lines under 80 characters in README files in > this repo. Fixed in patch set 3. > It would be nice to reformat this README as well unless you think it > would hurt readability. In cases of long links, one possible option is to > convert them to footnote-like links [...] Number-footnotes are kind of awful for maintenance unless automatically-generated. I did discover, however, that numeric identifiers are not necessary; text identifiers are also permitted. I used that syntax.
On 2017/09/11 10:44:33, Vasily Kuznetsov wrote: > Right now the folders in `codingtools` repository are called by the name of > the tool rather than by language. To stay consistent with this naming scheme, it > would make more sense to not have the `c++` folder here and place the README and > `clang-format-eyeo` file into a folder called `clang-format-eyeo` or something > like that. I find the navigability of the repository at present not optimal. I didn't recognize the names of some of the tools, since I don't work in those environments. I have to imagine the same will be true for others who are not familiar with C++. Our C++ group has talked about adding a number of other C++-specific tools. I can only imagine this will rapidly get more cluttered as we increase the level of automation and continue to add tools. It also seems that the convention of adding "-eyeo" to path names is redundant. Is there any doubt that this is an Eyeo repository? Do we really need the constant reminder? I used the naming convention in the configuration file, but after thinking about it more, I'd just as soon drop it. I'll defer to what you want to do. I care, but not that much.
On 2017/09/15 13:21:19, Eric wrote: > On 2017/09/11 10:44:33, Vasily Kuznetsov wrote: > > Right now the folders in `codingtools` repository are called by the name of > > the tool rather than by language. To stay consistent with this naming scheme, > it > > would make more sense to not have the `c++` folder here and place the README > and > > `clang-format-eyeo` file into a folder called `clang-format-eyeo` or something > > like that. > > I find the navigability of the repository at present not optimal. I didn't > recognize the names of some of the tools, since I don't work in those > environments. I have to imagine the same will be true for others who are not > familiar with C++. Our C++ group has talked about adding a number of other > C++-specific tools. I can only imagine this will rapidly get more cluttered as > we increase the level of automation and continue to add tools. The existing directory structure made sense initially as we had very few tools / configs. It would have been an overkill to separate them by language. I think rearrangement is still not necessary at this point, but if you add more C++-related tools, I would start leaning more towards grouping them under `c++` folder (and then perhaps following the same pattern for other languages / categories of tools in order to keep it consistent). > It also seems that the convention of adding "-eyeo" to path names is redundant. > Is there any doubt that this is an Eyeo repository? Do we really need the > constant reminder? I used the naming convention in the configuration file, but > after thinking about it more, I'd just as soon drop it. The `-eyeo` path names started with `flake8-eyeo`, which is a name of a Python package, so it's couldn't be just `flake8`. Same seems to be the case for `eslint-config-eyeo`, which is an NPM package. If it doesn't make sense in your case, I don't think there's any naming convention reason to add the `-eyeo` to the directory name. I agree with your point that we don't need an extra reminder. > I'll defer to what you want to do. I care, but not that much. Right now there are five directories under the root (including `c++`, which I propose to flatten and just have `clang-format` instead of it), so in my opinion it's not cluttered enough to start partitioning by language. On top of that, we only have one language-specific tool for each language, so if we partition, we'll end up with `c++/clang-format`, `js/eslint-config-eyeo` and `python/flake8-abp`. It seems like these directories that have only one directory inside of them are more an annoyance than help. My preference would be to keep things flat for now and to postpone the creation of language-specific directories until we have several tools for at least for one language. Does this sound reasonable?
Adding myself to CC 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
