search

sosy-lab / cpu-energy-meter

A tool for measuring energy consumption of Intel CPUs
BSD 3-Clause "New" or "Revised" License
321 stars 29 forks source link

Format code to a consistent style #17

Closed TBunk closed 6 years ago

TBunk commented 6 years ago

The style format is based on the linux kernel code style, which is well documented at https://www.kernel.org/doc/html/v4.10/ . The only exception is that the indentation is kept at 4 spaces (just as before), and a column limit is set, which is currently a maximum of 120 characters per line. The changes are most visible in the classes cpu-energy-meter.c and rapl.c

The program used for formatting the code was 'clang' with the following specifications: BasedOnStyle: LLVM IndentWidth: 4 UseTab: Always BreakBeforeBraces: Linux AllowShortIfStatementsOnASingleLine: false AllowShortLoopsOnASingleLine: false AllowShortFunctionsOnASingleLine: false IndentCaseLabels: false ColumnLimit: 120

TBunk commented 6 years ago

It seems this formatter uses tabs? This would be quite bad, please change it to use only spaces.

I guess this depends on the spaces of indendation (though, I havn't checked that yet). I.e., formatting the code with 4 spaces of indendation leads to tabs, whereas when I used 2 spaces today, the code was formatted with spaces (I have in fact double-checked that).

Could you please check if the diff gets significantly larger if we reformat everything to use 2 spaces of intendation (after applying all the other changes)? This is more common, but if this would increase the diff a lot, we could stay with 4 spaces.

I would imagine myself that the diff increases a lot, though, would that cause any problems for us? Otherwise, I would simply change it to 2 spaces in order to automatically solve the issue with tabs vs spaces (as stated above).

PhilippWendler commented 6 years ago

It seems this formatter uses tabs? This would be quite bad, please change it to use only spaces.

I guess this depends on the spaces of indendation (though, I havn't checked that yet). I.e., formatting the code with 4 spaces of indendation leads to tabs, whereas when I used 2 spaces today, the code was formatted with spaces (I have in fact double-checked that).

Very probably, the formatter currently uses tabs if the indentation is a multiple of 8. So changing the indentation to 2 does not really "fix" this, it just happens less often (any maybe never in our codebase).

Could you please check if the diff gets significantly larger if we reformat everything to use 2 spaces of intendation (after applying all the other changes)? This is more common, but if this would increase the diff a lot, we could stay with 4 spaces.

I would imagine myself that the diff increases a lot, though, would that cause any problems for us?

It's not a big problem of course, it is just that it makes using git blame much more tedious. But if you are in favor of 2 spaces, then do it.

mdangl commented 6 years ago

If @PhilippWendler's suspicion is correct, could you not simply extend the formatting process by adding a second step that replaces each with 8 spaces?

TBunk commented 6 years ago

I've found an option to exhibit the use of tabs, which I previously oversaw. This came quite in handy :-)

The indentation is now set to 2 spaces, however, git blame is now pretty much inaccurate (though, I for one don't mind). The column limit is now 100 characters per line, and the opening braces are now changed to be always attached to the surrounding context.

If it can add braces to ifs where they are missing, this would also be nice.

The formatting tool which I've used (=clang) doesn't have the option to add missing braces to if-statements. However, I can also add them by hand, as there aren't too many if-statements left where this is the case (around ~20).