Delete all trailing whitespaces and tabs to 2 spaces

[TobiKattmann]

Proposed Changes

Hi all,

this PR removes all trailing whitespaces from .cpp .hpp .inl .py .build files using
find . -type f -name '*.cpp' -exec sed --in-place 's/[[:space:]]\+$//' {} \+
in the code root

and all tabs are replaced by 2 spaces for .cpp .hpp .inl files using
find . -name '*.cpp' ! -type d -exec bash -c 'expand -t 2 "$0" > /tmp/e && mv /tmp/e "$0"' {} \;
This step of course can lead some misalignment if the person introducing the tab did not have tabs rendered as 2 spaces (e.g. 4 spaces per tab)

This is up of course for discussion and can be seen as a test vehicle to get everyones discussion. One can try to merge this to check if merge conflicts are really that bad to handle. Of course there will be conflicts on lines you touched as well. A possible solution to merge conflicts is to apply the commands to your filebase and then merge... then it is quite likely that your only merge conflicts are the ones you have with develop anyway.

The rough plan discussed with @talbring is to include automatic checking of trailing whitespaces, tabs, overlong lines and possibly other stuff.

Cheers 🍾 to @WallyMaier who seemed to have to have that on his agenda for quite some time.

Edit: Only the restructured Ccpp Chpp C*inl files are currently cleaned to keep the commit size down. This could be extended to other file chunks in the future. See externals/utils/replace-tabs...sh for a little shell script to get the receive the same results as in this PR.

PR Checklist

• I am submitting my contribution to the develop branch.
• My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags).
• My contribution is commented and consistent with SU2 style. -> now it should be a bit more
• I have added a test case that demonstrates my contribution, if necessary. not necessary

[pcarruscag]

Someone beat you to it @WallyMaier

@TobiKattmann there is no doubt we need to do this, I dream of the day I can turn on the "auto-strip" on the IDE I use.
However I do not agree with doing it this way :)
This creates a lot of entropy while merging for no obvious benefit, besides, this is already being done on the files we restructure...
Please restrict the scope of this PR to files that have been restructured (and possibly need a maintenance pass), or to files that you are willing to restructure yourself, that includes:

• One class per file (unless they are tiny I guess);
• Moving the contents of the .inl files to the corresponding .hpp;
• Revising includes to reduce compilation dependencies.

The above is more or less what we agreed in #583.
In #807 I suggested starting with geometry classes as I guess fewer people are working on them. I further ask you to open the PR as draft and announce what you want to restructure to minimize conflicts for folks who might have a lot of developments in certain files.

[talbring]

Actually I am in favor of doing it once and for all otherwise it will be a never-ending story. Like @TobiKattmann already mentioned, it should not lead to a lot of problems as long as people first remove/replace white-spaces/tabs in their branches (or use auto-strip like you mentioned). Compared to the restructuring the work is negligible.

[pcarruscag]

Yes but it makes it harder to compare your current work to previous versions, as all you would get is blank lines.
All I am saying is, if we are going to create entropy let's do things properly and do the restructuring we planned.
Splitting files does a lot more for readability than just removing white spaces, and by doing this work in chunks you actually get a chance of fixing other (more important) formatting and commenting issues, doing it like this you probably introduce more of those (but we would never know since ain't nobody got time to scroll through 50k changes).
But like I said before, I am all for cleaning files that have been restructured already and may have accumulated white spaces since then.

[talbring]

It is also possible to ignore white-spaces for diffs (also here on github) so I don't see a big burden. However, of course if the majority agrees with not doing it at the moment we will follow that opinion.

Still, I will set up code factor to complain about new trailing white-spaces, line-lengths and tabs.

[TobiKattmann]

harder to compare your current work to previous versions

git diff --ignore-space-change -> the diff to develop is empty

Splitting files does a lot more for readability than just removing white spaces

True... removing white spaces actually doesn't do much for readability. And this is not about "one or the other". Restructuring files with one class per file + consistent naming + checking comments etc has a lot more value.... and can still be done nonethless

fixing other (more important) formatting and commenting issues,

New Formatting issues wrt to the tab removal are not introduced, they are just different now ... and consistent. With tabs it depends on the IDE setup whether the formatting for every specific line is ok. Because tabs are prob introduced by people with different space-per-tab ratio.

The benefit I see is the possibility of a "once-and-for-all" solution of this PR in combination with some kind of trailing-whitespaces+tabs-bot.

[pcarruscag]

What about running the solution you propose only on subfolders? i.e. on src/something/ but not on src/
As we restructure and move files to subfolders they are handled automatically... best of both worlds.

[talbring]

Just as an information: I enabled CodeFactor to check the pull requests for tabs and white-spaces (I added CPPLINT.cfg file in the develop branch). However, this configuration does not work properly for PRs that are already open, they still use the default config. That is the reason why it is complaining about "Redundant blank line .." For new PRs it should work properly.

see comment

[pcarruscag]

So, all my old branches have more important merge issues than this, so if you guys think this is the way to go... then go for it. @TobiKattmann your second command replaces tabs by 4 spaces not 2.
I still don't think changing 50k lines in one PR is a good practice (I hope by saying this I jinx myself and all ends up going smoothly).

[economon]

I am open to this (we have discussed it for a couple of years), and it would be good to put it into a script like externals/fix_line_endings.sh. Could even be automated later somehow with that on PRs, or can at least run manually to appease CodeFactor.

I think you guys were going to run some tests on branches.. please let folks know what you see and what the steps should be

[TobiKattmann]

Hey @pcarruscag

What about running the solution you propose only on subfolders

Sure, I would be ok with that. If everyone agrees on this, then it is better to do it in steps than not. And of course If there is good evidence that it messes up peoples merges than we better not do it.

your second command replaces tabs by 4 spaces not 2

oh right, I need to check whether I ran the command with 2 spaces and copied the 4 spaces command over from stack overflow. I changed the command for now. Hmmm...looking at the commit I am rather certain that it is 1tab=2spaces in the commit I did.

I still don't think changing 50k lines in one PR is a good practice

Me neither. It also kindof makes git blame harder to use on those lines. If someone has a nicer way then I am keen to hear that

@economon

good to put it into a script like externals/fix_line_endings.sh [...] I think you guys were going to run some tests on branches

I will prepare both over the weekend and commit/report :)

Cheers all, Tobi

[dkavolis]

Why not simply use clang-format and have a script to pass files/directories to have formatted? It provides more formatting options than just stripping trailing whitespaces and replacing tabs and does it in a consistent way.

[pcarruscag]

Why not simply use clang-format and have a script to pass files/directories to have formatted? It provides more formatting options than just stripping trailing whitespaces and replacing tabs and does it in a consistent way.

Tried that on a couple of files out of curiosity, it does not look very good, for example we have the habit of aligning repetitive statements across multiple lines clang-format does not keep that, we have very long lines with chained methods that look awkward when broken up.
Clang probably has a neater architecture of tiny objects where auto formatting works very well.

[dkavolis]

Tried that on a couple of files out of curiosity, it does not look very good, for example we have the habit of aligning repetitive statements across multiple lines clang-format does not keep that, we have very long lines with chained methods that look awkward when broken up.
Clang probably has a neater architecture of tiny objects where auto formatting works very well.

Most of it might be just a case of setting the correct option values in .clang-format file.
Line breaks can be disabled by setting ColumnLimit to 0, this also preserves any existing line breaks.

You can try out https://gist.github.com/dkavolis/d240b0db70e21a3f33964d0a289f82f1
I'd say the code looks much better even if in a few places you lose aligned repetitive statements which are hard to maintain manually anyway.

[TobiKattmann]

Hi all,

After the initial excitement of clearing all tabs and trailing whitespaces... I guess it is more reasonable to follow @pcarruscag proposal:

What about running the solution you propose only on subfolders? i.e. on src/something/

I now trimmed all C*.cpp, C*.hpp and C*.inl files in SU2_CFD which is equivalent to all restructured files in the Sub-folders. I added a basic script replace-tabs-...sh in externals/utils which provides this functionality. I would enhance that script if this is the way to go.
The commit size now shrunk down to ~4k changed lines. Possible merge problems will be much smaller.

I found to have to no problem when merging the develop first -> clearing all tabs/whitespaces in the feature_branch with the provided script -> merge this develop_noWhitespaces using the --strategy-option=ours option. Merge conflicts will be purely due to tabs/whitespaces therefore one always wants the own code in case of conflict, as all conflicts with the develop related to other stuff were already resolved in the first merge.

Now that the commit is a lot smaller, there should be even less problems. Maybe some folks will have no problems at all.

After some back and forth in the commits I briefly chatted with @talbring to do a git rebase / squash to not have these huge commits in the history. Otherwise one could open a new & clean PR if we can agree on an approach here to keep the discussion in one place.

[pcarruscag]

Thanks for that. I would go with clean PR as I am not that git-savvy, what ever is less work.

By the way, I am now fixing indentation issues anywhere my normal development takes me (probably not very good for PR tidiness but if we want to clean things up we need to start somewhere).

[talbring]

I agree that this is a good first step.

Thanks for that. I would go with clean PR as I am not that git-savvy, what ever is less work.

By the way, I am now fixing indentation issues anywhere my normal development takes me (probably not very good for PR tidiness but if we want to clean things up we need to start somewhere).

I think that's fine. Lets do it like that as we go.

[pcarruscag]

LGTM