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

Cleanup code #9

Closed PhilippWendler closed 6 years ago

PhilippWendler commented 7 years ago

The code base still contains a lot of old code that is now unused, and should be removed. Probably there could also be some refactorings that simplify the remaining code then.

TBunk commented 6 years ago

@PhilippWendler I'd deem the code quality in a pretty good state right now. I've removed nearly all of the unused code in previous commits (e.g., see commit bd08d71ee7b86b492faedfe22d78fcd59599b69a and aa70eadfe0ef00976d64ac73d4540385a1c69371), with some deliberate exceptions. The excessive use of error-values (int) as returning values seems a bit odd to me, though it is well and consistently coded, and working as intended (i.e., nothing that needs to be refactored imo).

The only thing left to do would be the use of a code formatter for a consistent code base. Do you agree with me here? I would use the linux kernel code style format (see https://www.kernel.org/doc/html/v4.10/ process/coding-style.html), because afaik that's the format that the code was previously based on (partially, at least).

PhilippWendler commented 6 years ago

I don't like the kernel coding style very much (indentation is 8 spaces). If the code is only a little bit inconsistent, we could keep it and just improve new and changed code. But if you want, you can of course also reformat it. I just would prefer to keep indentation at 2 spaces.

TBunk commented 6 years ago

I forgot to mention this in my last reply, but my intention here is to keep the indentation as it is (but to set a max. column limit, though!). I will give you a preview of the formatted code in the upcoming week, then we can discuss whether the outcome seems expedient or not.

As for the issue itself, I think that the code is in a pretty good state. I'm open for any further suggestions, but otherwise I'll leave the code as it is for now.

TBunk commented 6 years ago

The code was formatted in #17 . For reference, I've used clang with a .clang-format file that contained the following specifications: BasedOnStyle: LLVM IndentWidth: 2 UseTab: Never BreakBeforeBraces: Attach AllowShortIfStatementsOnASingleLine: false AllowShortLoopsOnASingleLine: false AllowShortFunctionsOnASingleLine: false IndentCaseLabels: false ColumnLimit: 100