Re: [Powertop] [RFC][PATCH 2/2] Convert powertop to use new report generator facility

[Chris Ferron <chris.e.ferron at linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2594 bytes --]

On 10/03/2012 10:06 AM, Igor Zhbanov wrote:
> Convert report system to use new report generator facility
> for producing parsable CSV- and HTML-reports, enhancing code readability
> and maintainability, merging CSV and HTML codepaths, separating document
> structure from text formatting.
> ---
>   src/cpu/cpu.cpp            |  410 ++++++++++----------------------------------
>   src/cpu/cpu.h              |    5 +
>   src/cpu/cpu_core.cpp       |   11 --
>   src/cpu/cpu_linux.cpp      |   55 +++++--
>   src/cpu/cpu_package.cpp    |   12 --
>   src/cpu/intel_cpus.cpp     |   19 --
>   src/devices/device.cpp     |  112 ++++--------
>   src/devlist.cpp            |   44 ++----
>   src/main.cpp               |   13 +-
>   src/powertop.css           |   10 +
>   src/process/do_process.cpp |  199 ++++++++++------------
>   src/report.cpp             |  145 +++++-----------
>   src/report.h               |   12 +-
>   src/tuning/tuning.cpp      |  145 +++++-----------
>   14 files changed, 393 insertions(+), 799 deletions(-)
>
>
Hello,
So first id like to say nice work. I see lots of improvements, and some 
fixes that were missed in the past.
Couple of things,

#1 I noticed that the scripts section of Software Settings in need of 
Tuning is missing. I see the nice header, but not data.
#2 C0 active table cells are not aligned , and the CPU's are also not 
aligned correctly in the Processor Idle state report section.
#3 I get some warning compiling, I know PowerTOP is not warning less, 
but would like to attempt to prevent adding more. (and fixing exiting 
ones of course)
report/report-maker.cpp: In destructor ‘report_maker::~report_maker()’:
report/report-maker.cpp:51:10: warning: deleting object of abstract 
class type ‘report_formatter’ which has non-virtual destructor will 
cause undefined behaviour [-Wdelete-non-virtual-dtor]
report/report-maker.cpp: In member function ‘void 
report_maker::setup_report_formatter()’:
report/report-maker.cpp:110:10: warning: deleting object of abstract 
class type ‘report_formatter’ which has non-virtual destructor will 
cause undefined behaviour [-Wdelete-non-virtual-dtor]

#4 report[c,h] should probable just go in your new sub/dir (report).
#5 csv report has alignment issues for both the cpu Idle and Freq. Even 
if they are machine readable, they need to be easily parsed using open 
office or MS excel and look aligned with the http report.

other issues are small and can be attended to in the future, or have 
been already expressed.

Thanks
-C