Skip to content
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

Space the final frontier #1893

Merged
merged 142 commits into from
Aug 19, 2024

Conversation

stefanrueger
Copy link
Collaborator

This PR reformats most of the C sources and removes tabs from some other files for consistent style.

There are some disadvantages of this, eg, diffs within git across this PR are going to be neigh impossible (though larger pieces of code changes in the recent past have already made diffs over a wide time span difficult). Weighing up improved maintenance (adding lines in files no longer poses minefields of working with sprinkles of tabs), enhanced readability through consistent code display, and modern debugging techniques (zooming in on commits that introduce bugs) that no longer require diffs over long time intervals has convinced me that this PR is beneficial.

@ndim
Copy link
Contributor

ndim commented Aug 18, 2024

This is a HUGE change and therefore intended for post avrdude-8.0, I presume?

IFF you want to enforce a certain coding style (and that is a VERY BIG IFF), could you please also add some tooling to consistently maintain that style for the future: Editor settings and formatting tool settings and git hooks for local use before pushing a commit, and CI checks which ensures mistakes in PRs are caught before merging them.

Without that, the code inconsistency will inevitably increase again over time and the inconvenience this huge commit causes will have been wasted effort.

Also, while the .editorconfig does contain some information regarding whitespace use, it does not completely define the coding style, not even just regarding whitespace.

I still find confusingly inconsistent code formatting: switch (expr) and switch(expr) with and without space , if (expr) and if(expr), while(expr) and while (expr). None of those are formally defined in .editorconfig, and I cannot even find a human readable verbal description on coding style in the git repo.

I know my personal preferences (mostly Linux Kernel style, with exceptions e.g. for MISRA C style always { } after if), but IF you do such a big change, please do it consistently.

Exceptions to the consistent coding style should be made for source files which are copies of code from somewhere else, e.g. src/msvc/*, and those exceptions should be formally specified.

BTW: what tool did you use with which settings? That could be helpful for the tooling.

Of course, while automatic code formatting tools do ensure consistency, they do not necessarily make things more readable.

I personally would first verbally define a coding style and add that description to the git repo, then formally define it as configuration for one or several tools (tools should work on BSD, Linux, MacOS and Windows), and only then apply and enforce it. This PR only applies an undefined set of rules, some of which might be formally specified in .editorconfig, and some of which are not specified neither formally nor verbally.

Some discussion on coding style has happened on #1572, but I have not found that written down or specified formally anywhere in the avrdude git repo.

@stefanrueger
Copy link
Collaborator Author

stefanrueger commented Aug 18, 2024

This is a HUGE change and therefore intended for post avrdude-8.0, I presume?

Au contraire! This PR effects zero change in the sense below. It is safe to merge before v8.0.

The created avrdude executable stays invariant except for the source line numbers that errors and warnings emit with -vv. To prove this check out git main, replace all occurrences of __LINE__ in avrdude.h and jtagmkII.c with 07654321. Compile the executable avrdude and disassemble using objdump -d $(which avrdude) >ad.S. Then do the same with this PR and convince yourself that the ensuing .S files are identical. OK, I only checked under Linux but have every reason to believe that the semi-automated method of this PR will also leave the fraction of the code invariant that isn't seen by a Linux compilation.

still find confusingly inconsistent code formatting: switch (expr) and switch(expr)

A hairy hand perhaps? 😉

Yes, indent -as -nsaf -nsai -nsaw -nut -bap -bacc -bad -bbb -brs -nbc -br -brf -nbs -c33 -cd33 -cp33 -ncdb -cdw -ce -ci2 -cli0 -d0 -di1 -neei -nfc1 -i2 -il-2 -ppi0 -nip -l130 -lc130 -nlp -npcs -npsl -nsc -sob -nlps -nprs -par might have missed that space. It has a few other quirks that I polish with a set of sed scripts to ensure the correct spacing of a? b: c and type *p, single-line comments just so // Note the space and capital letter at the start and

/*
 * Multi-line comments which
 *  - Tend to be about larger code regions
 *  - Contain vital algorithmic information
 *  - Deserve visual and prominence
 *  - Help users utilise functions and code thereafter
 */

This has not completely been automated. I am sure there are better tools but, as they say, perfect is the enemy of good

It is probably not possible to make .editorconfig reflect that.

coding style has happened on #1572, but I have not found that written down or specified formally anywhere

Correct. There is none, and I don't think the project has an aspiration of being prescriptive or trying to enforce style.

What I found, however, is that contributors, say for a new programmer, tend to copy code for a similar programmer and then morph that into something that works. This perpetuates pain for maintenance. This PR will ensure that people start from a halfway reasonable style.

So, all things considered, whilst I acknowledge that this PR might cause inconvenience, and that there will be deviations in the future, I still consider this PR a good investment into maintainability. And for the record: I am asking everyone who spent energy and thought on the style they used for their contribution to forgive this PR for riding roughshot over that.

Exceptions [...] should be made for source files which are copies of code from somewhere else, e.g. src/msvc/*

Agreed, and I don't think this PR has touched those. Let me know specific examples where this PR has cast the net too wide.

I plan to push a couple more commits onto this PR that are avrdude-specific and improve readability

  • Change PDATA(pgm)-> to my.
  • Change xxx->prog_modes & PM_ABCD to is_abcd(xxx)

Again, this will not change the executable.

@mcuee mcuee added the enhancement New feature or request label Aug 19, 2024
@stefanrueger stefanrueger merged commit 81d15b5 into avrdudes:main Aug 19, 2024
12 checks passed
@stefanrueger stefanrueger deleted the space-the-final-frontier branch August 19, 2024 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants