ArduPilot / ardupilot

ArduPlane, ArduCopter, ArduRover, ArduSub source
http://ardupilot.org/
GNU General Public License v3.0
10.7k stars 17.16k forks source link

DevEnv: Finish out the astylerc settings #19009

Open TunaLobster opened 2 years ago

TunaLobster commented 2 years ago

Feature request

Problem: Currently the astylerc file works well for some files, but it does not work well for headers. It also does not include all of our formatting guidelines causing some odd results when using it and not following all of the guidelines at the start.

Solution: Going through the long list of defaults that astyle implements and correcting the ones that need to be changed. Adding in checking for header files.

Platform [x] All [ ] AntennaTracker [ ] Copter [ ] Plane [ ] Rover [ ] Submarine

khancyr commented 2 years ago

the issue with the formatter is that we aren't conform to our own style on some files for readability so, I doubt it will work. But it could be nice to have a little update on it. Another linter to consider would be clang-format as it is well used on IDE.

Rohan-Dah commented 1 year ago

Hi there folks... is this issue still open? I would love to work on it but I'll need a bit of guidance as it will be my first issue..

TunaLobster commented 1 year ago

I think I might have done that here. This isn't merged so nothing has been completed really. https://github.com/TunaLobster/ardupilot/compare/pr/udpate-pre-commit...TunaLobster:ardupilot:pre/add-astyle-pre-commit

There might be a few more optional astyle things that I missed while reading through the docs. Like khancyr said, if there is a better linter for us, that would work too.

Rohan-Dah commented 1 year ago

So I went through the style documentation page and understood what astyle is and how it is being used in the directory. This is the link of what I referred https://ardupilot.org/dev/docs/style-guide.html

Here I found below things that can be fixed here -> ardupilot/Tools/CodeStyle/astylerc 1) 'Trailing whitespaces' that needs fixing. ->As referred above, a linter can be used here to fix this. Clang tidy can be used here serving as a linter.

2) Line Breaks: Single statements (Each statement should get its own line) -> This can be done by setting 'keep-one-line-statements' as true.

3) Braces: Function definitions: place each brace on its own line. For methods inside a header file, braces can be inline. -> A linter can play its role here.

Control statements (if, while, do, else) should always use braces around the statements -> It can be done by setting value of 'add-brackets' as true

Other Braces: -> Fix for this too can be linter

4) For 'Names', 'Functions and variables' and rest of the page there aren't specific options in astyle that can be used however it could be manually named as required.

Ryanf55 commented 9 months ago

I found a major problem with the astyle setting. https://github.com/ArduPilot/ardupilot/pull/25579#discussion_r1402717914

We're using astyle in parts of the codebase and enforcing it in CI. But, it's doing nonsensical things.

Did anyone have a plan for why the astyle config is added to the repo and used, but it's not apparently finished?

khancyr commented 9 months ago

astyle was there years ago, nobody enforced it nor make a clean configuration. We can switch to another tool if we can have some consitent. Main issue is that we cannot make a first pass to clean the full repo as that breaks all PRs and history. Second one, is that on some part of the code we are doing manual indention for "easier reading" and linter cannot cope with those

Ryanf55 commented 9 months ago

Ok. If astyle is known to be broken, perhaps we need to remove it from the repo and remove all tooling using it.

clang-tidy supports adding ignores, so you can do manual indent and tell the linter not to touch it. Or, configure the linter to manually indent your matrices.

See this PR for a partial solution to the history problem: https://github.com/ArduPilot/ardupilot/pull/25609

Ryanf55 commented 9 months ago

For the lack of correct formatting in a struct with brace initialization, upgrading to astyle 3.2 should fix it. Ubuntu 22 comes with 3.1. https://astyle.sourceforge.net/notes.html fixed struct bitfield indentation (#534) https://sourceforge.net/p/astyle/bugs/534/