Small rework on report formatter side

Hello,

below is initial version of rework I was talking about. Igor did most part of it (introduced report_formatter_base class), so mine are just minor tweaks. I'm glad still no one mentioned templates, just kidding.

one thing I don't really like so far is

        else if (type == REPORT_OFF)
               formatter = new report_formatter();

which is just cryptic and thus look wrong.
perhaps we need something like `new report_formatter_basic()' here (which will require header file rename).

please review and comment.

------------------------------------------------------------------------

Small rework on report formatter side:

- make report_formatter a basic class (previously was interface). hence we can remove empty report_formatter interface implementation.

- drop report-formatter-null class

- in report maker now we create report_formatter instance for REPORT_OFF mode

- rename report_formatter_base to report_formatter_string_base, so class name tells a bit more, later we can create report_formatter_xml_base/report_formatter_json_base/etc.

Signed-off-by Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>

Quoted 26 lines · Sergey Senozhatsky

Hello,

below is initial version of rework I was talking about. Igor did most part of it (introduced report_formatter_base class), so mine are just minor tweaks. I'm glad still no one mentioned templates, just kidding.

one thing I don't really like so far is

         else if (type == REPORT_OFF)
                formatter = new report_formatter();

which is just cryptic and thus look wrong.
perhaps we need something like `new report_formatter_basic()' here (which will require header file rename).

please review and comment.

------------------------------------------------------------------------

Small rework on report formatter side:

- make report_formatter a basic class (previously was interface). hence we can remove empty report_formatter interface implementation.

- drop report-formatter-null class

Quoted 8 lines

- in report maker now we create report_formatter instance for REPORT_OFF mode

- rename report_formatter_base to report_formatter_string_base, so class name tells a bit more, later we can create report_formatter_xml_base/report_formatter_json_base/etc.

Signed-off-by Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>

plus:

-- renamed report_formatter to basic_report_formatter, so now

          else if (type == REPORT_OFF)
                 formatter = new basic_report_formatter();

look a bit better (yeah, we have basic_report_formatter and report_formatter_string_base. ugly!!!)

-- removed some empty function from CSV/HTML classes. like

	void report_formatter_csv::add_empty_cell()
	{
		/* Do nothing special */
	}

we don't really need such functions in derived classes, they're already doing nothing in base class.

---
  src/Makefile.am                      |   1 -
  src/report/basic-report-formatter.h  |  65 +++++++++++++
  src/report/report-formatter-base.cpp |  14 +--
  src/report/report-formatter-base.h   |   4 +-
  src/report/report-formatter-csv.cpp  |  42 --------
  src/report/report-formatter-csv.h    |  10 +-
  src/report/report-formatter-html.h   |   2 +-
  src/report/report-formatter-null.cpp | 179 -----------------------------------
  src/report/report-formatter-null.h   |  66 -------------
  src/report/report-formatter.h        |  65 -------------
  src/report/report-maker.cpp          |   3 +-
  src/report/report-maker.h            |   4 +-
  12 files changed, 79 insertions(+), 376 deletions(-)

src/Makefile.am +0 -1

index ccf3f0c..1a64622 100644
@@ -34,7 +34,6 @@ powertop_SOURCES = parameters/persistent.cpp parameters/learn.cpp parameters/par
  		report/report-formatter-base.cpp report/report-formatter-base.h \
  		report/report-formatter-csv.cpp report/report-formatter-csv.h \
  		report/report-formatter-html.cpp report/report-formatter-html.h \
-		report/report-formatter-null.cpp report/report-formatter-null.h \
  		main.cpp css.h powertop.css cpu/intel_gpu.cpp
 
 

src/report/basic-report-formatter.h new file +65 -0

new file mode 100644
index 0000000..5134bf5
@@ -0,0 +1,65 @@
+/* Copyright (c) 2012 Samsung Electronics Co., Ltd.
+ *	http://www.samsung.com/
+ *
+ * This file is part of PowerTOP
+ *
+ * This program file is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program in a file named COPYING; if not, write to the
+ * Free Software Foundation, Inc,
+ * 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301 USA
+ * or just google for it.
+ *
+ * report_formatter interface.
+ * Written by Igor Zhbanov <i.zhbanov(a)samsung.com>
+ * 2012.10 */
+
+#ifndef _REPORT_FORMATTER_H_
+#define _REPORT_FORMATTER_H_
+
+#include "report-maker.h"
+
+class basic_report_formatter
+{
+public:
+	virtual ~basic_report_formatter() {}
+
+	virtual void finish_report() {};
+	virtual const char *get_result() {return "Basic report_formatter::get_resul call\n";};
+	virtual void clear_result() {};
+
+	virtual void add(const char *str) {};
+	virtual void addv(const char *fmt, va_list ap) {};
+
+	virtual void add_header(const char *header, int level) {};
+
+	virtual void begin_section(section_type stype) {};
+	virtual void end_section() {};
+
+	virtual void begin_table(table_type ttype) {};
+	virtual void end_table() {};
+
+	virtual void begin_row(row_type rtype) {};
+	virtual void end_row() {};
+
+	virtual void begin_cell(cell_type ctype) {};
+	virtual void end_cell() {};
+	virtual void add_empty_cell() {};
+
+	virtual void begin_paragraph() {};
+	virtual void end_paragraph() {};
+
+	/* For quad-colouring CPU tables in HTML */
+	virtual void set_cpu_number(int nr) {};
+};
+
+#endif /* _REPORT_FORMATTER_H_ */

src/report/report-formatter-base.cpp +7 -7

index 4b448be..c6d5d9d 100644
@@ -36,7 +36,7 @@
  /* ************************************************************************ */
 
  const char *
-report_formatter_base::get_result()
+report_formatter_string_base::get_result()
  {
  	return result.c_str();
  }

@@ -44,7 +44,7 @@ report_formatter_base::get_result()
  /* ************************************************************************ */
 
  void
-report_formatter_base::clear_result()
+report_formatter_string_base::clear_result()
  {
  	result.clear();
  }

@@ -52,7 +52,7 @@ report_formatter_base::clear_result()
  /* ************************************************************************ */
 
  void
-report_formatter_base::add(const char *str)
+report_formatter_string_base::add(const char *str)
  {
  	assert(str);
 

@@ -62,7 +62,7 @@ report_formatter_base::add(const char *str)
  /* ************************************************************************ */
 
  void
-report_formatter_base::add_exact(const char *str)
+report_formatter_string_base::add_exact(const char *str)
  {
  	assert(str);
 

@@ -74,7 +74,7 @@ report_formatter_base::add_exact(const char *str)
  #define LINE_SIZE 8192
 
  void
-report_formatter_base::addv(const char *fmt, va_list ap)
+report_formatter_string_base::addv(const char *fmt, va_list ap)
  {
  	char str[LINE_SIZE];
 

@@ -87,7 +87,7 @@ report_formatter_base::addv(const char *fmt, va_list ap)
  /* ************************************************************************ */
 
  void
-report_formatter_base::addf(const char *fmt, ...)
+report_formatter_string_base::addf(const char *fmt, ...)
  {
  	va_list ap;
 

@@ -101,7 +101,7 @@ report_formatter_base::addf(const char *fmt, ...)
  /* ************************************************************************ */
 
  void
-report_formatter_base::addf_exact(const char *fmt, ...)
+report_formatter_string_base::addf_exact(const char *fmt, ...)
  {
  	char str[LINE_SIZE];
  	va_list ap;

src/report/report-formatter-base.h +2 -2

index 7063174..61cf1ba 100644
@@ -26,9 +26,9 @@
  #ifndef _REPORT_FORMATTER_BASE_H_
  #define _REPORT_FORMATTER_BASE_H_
 
-#include "report-formatter.h"
+#include "basic-report-formatter.h"
 
-class report_formatter_base: public report_formatter
+class report_formatter_string_base: public basic_report_formatter
  {
  public:
  	virtual const char *get_result();

src/report/report-formatter-csv.cpp +0 -42

index 9b154b5..893c92a 100644
@@ -45,14 +45,6 @@ report_formatter_csv::report_formatter_csv()
  /* ************************************************************************ */
 
  void
-report_formatter_csv::finish_report()
-{
-	/* Do nothing special */
-}
-
-/* ************************************************************************ */
-
-void
  report_formatter_csv::add_doc_header()
  {
  	add_header(report_csv_header, 1);

@@ -72,24 +64,6 @@ report_formatter_csv::add_header(const char *header, int level)
  	add_exact("\n");
  }
 
-/* ************************************************************************ */
-
-void
-report_formatter_csv::begin_section(section_type stype)
-{
-	/* Do nothing special */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_csv::end_section()
-{
-	/* Do nothing special */
-}
-
-/* ************************************************************************ */
-
  void
  report_formatter_csv::begin_table(table_type ttype)
  {

@@ -163,14 +137,6 @@ report_formatter_csv::end_cell()
  /* ************************************************************************ */
 
  void
-report_formatter_csv::add_empty_cell()
-{
-	/* Do nothing special */
-}
-
-/* ************************************************************************ */
-
-void
  report_formatter_csv::begin_paragraph()
  {
  	text_start = result.length();

@@ -213,11 +179,3 @@ report_formatter_csv::escape_string(const char *str)
 
  	return res;
  }
-
-/* ************************************************************************ */
-
-void
-report_formatter_csv::set_cpu_number(int nr UNUSED)
-{
-	/* Do nothing */
-}

src/report/report-formatter-csv.h +1 -9

index fbec0c9..8db9839 100644
@@ -44,18 +44,13 @@
 
  /* ************************************************************************ */
 
-class report_formatter_csv: public report_formatter_base
+class report_formatter_csv: public report_formatter_string_base
  {
  public:
  	report_formatter_csv();
 
-	void finish_report();
-
  	void add_header(const char *header, int level);
 
-	void begin_section(section_type stype);
-	void end_section();
-
  	void begin_table(table_type ttype);
  	void end_table();
 

@@ -64,13 +59,10 @@ public:
 
  	void begin_cell(cell_type ctype);
  	void end_cell();
-	void add_empty_cell();
 
  	void begin_paragraph();
  	void end_paragraph();
 
-	void set_cpu_number(int nr);
-
  private:
  	void add_doc_header();
 

src/report/report-formatter-html.h +1 -1

index 1c3eb4a..4d00efe 100644
@@ -55,7 +55,7 @@ struct html_cell {
 
  /* ************************************************************************ */
 
-class report_formatter_html: public report_formatter_base
+class report_formatter_html: public report_formatter_string_base
  {
  public:
  	report_formatter_html();

src/report/report-formatter-null.cpp deleted +0 -179

deleted file mode 100644
index 22da7a0..0000000
@@ -1,179 +0,0 @@
-/* Copyright (c) 2012 Samsung Electronics Co., Ltd.
- *	http://www.samsung.com/
- *
- * This file is part of PowerTOP
- *
- * This program file is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; version 2 of the License.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
- * for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program in a file named COPYING; if not, write to the
- * Free Software Foundation, Inc,
- * 51 Franklin Street, Fifth Floor,
- * Boston, MA 02110-1301 USA
- * or just google for it.
- *
- * Null report generator.
- * Written by Igor Zhbanov <i.zhbanov(a)samsung.com>
- * 2012.10 */
-
-#include <stdarg.h>
-
-#include "report-formatter-null.h"
-
-/* ************************************************************************ */
-
-report_formatter_null::report_formatter_null()
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::finish_report()
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-const char *
-report_formatter_null::get_result()
-{
-	return "";
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::clear_result()
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::add(const char *str UNUSED)
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::addv(const char *fmt UNUSED, va_list ap UNUSED)
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::add_header(const char *header UNUSED, int level UNUSED)
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::begin_section(section_type stype UNUSED)
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::end_section()
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::begin_table(table_type ttype UNUSED)
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::end_table()
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::begin_row(row_type rtype UNUSED)
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::end_row()
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::begin_cell(cell_type ctype UNUSED)
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::end_cell()
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::add_empty_cell()
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::begin_paragraph()
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::end_paragraph()
-{
-	/* Do nothing */
-}
-
-/* ************************************************************************ */
-
-void
-report_formatter_null::set_cpu_number(int nr UNUSED)
-{
-	/* Do nothing */
-}

src/report/report-formatter-null.h deleted +0 -66

deleted file mode 100644
index 51493ee..0000000
@@ -1,66 +0,0 @@
-/* Copyright (c) 2012 Samsung Electronics Co., Ltd.
- *	http://www.samsung.com/
- *
- * This file is part of PowerTOP
- *
- * This program file is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; version 2 of the License.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
- * for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program in a file named COPYING; if not, write to the
- * Free Software Foundation, Inc,
- * 51 Franklin Street, Fifth Floor,
- * Boston, MA 02110-1301 USA
- * or just google for it.
- *
- * Null report generator.
- * Written by Igor Zhbanov <i.zhbanov(a)samsung.com>
- * 2012.10 */
-
-#ifndef _REPORT_FORMATTER_NULL_H_
-#define _REPORT_FORMATTER_NULL_H_
-
-#include "report-formatter.h"
-
-/* ************************************************************************ */
-
-class report_formatter_null: public report_formatter
-{
-public:
-	report_formatter_null();
-
-	void finish_report();
-	const char *get_result();
-	void clear_result();
-
-	void add(const char *str);
-	void addv(const char *fmt, va_list ap);
-
-	void add_header(const char *header, int level);
-
-	void begin_section(section_type stype);
-	void end_section();
-
-	void begin_table(table_type ttype);
-	void end_table();
-
-	void begin_row(row_type rtype);
-	void end_row();
-
-	void begin_cell(cell_type ctype);
-	void end_cell();
-	void add_empty_cell();
-
-	void begin_paragraph();
-	void end_paragraph();
-
-	void set_cpu_number(int nr);
-};
-
-#endif /* _REPORT_FORMATTER_NULL_H_ */

src/report/report-formatter.h deleted +0 -65

deleted file mode 100644
index cec0796..0000000
@@ -1,65 +0,0 @@
-/* Copyright (c) 2012 Samsung Electronics Co., Ltd.
- *	http://www.samsung.com/
- *
- * This file is part of PowerTOP
- *
- * This program file is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; version 2 of the License.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
- * for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program in a file named COPYING; if not, write to the
- * Free Software Foundation, Inc,
- * 51 Franklin Street, Fifth Floor,
- * Boston, MA 02110-1301 USA
- * or just google for it.
- *
- * report_formatter interface.
- * Written by Igor Zhbanov <i.zhbanov(a)samsung.com>
- * 2012.10 */
-
-#ifndef _REPORT_FORMATTER_H_
-#define _REPORT_FORMATTER_H_
-
-#include "report-maker.h"
-
-class report_formatter /* Interface */
-{
-public:
-	virtual ~report_formatter() {}
-
-	virtual void finish_report() = 0;
-	virtual const char *get_result() = 0;
-	virtual void clear_result() = 0;
-
-	virtual void add(const char *str) = 0;
-	virtual void addv(const char *fmt, va_list ap) = 0;
-
-	virtual void add_header(const char *header, int level) = 0;
-
-	virtual void begin_section(section_type stype) = 0;
-	virtual void end_section() = 0;
-
-	virtual void begin_table(table_type ttype) = 0;
-	virtual void end_table() = 0;
-
-	virtual void begin_row(row_type rtype) = 0;
-	virtual void end_row() = 0;
-
-	virtual void begin_cell(cell_type ctype) = 0;
-	virtual void end_cell() = 0;
-	virtual void add_empty_cell() = 0;
-
-	virtual void begin_paragraph() = 0;
-	virtual void end_paragraph() = 0;
-
-	/* For quad-colouring CPU tables in HTML */
-	virtual void set_cpu_number(int nr) = 0;
-};
-
-#endif /* _REPORT_FORMATTER_H_ */

src/report/report-maker.cpp +1 -2

index 4a68a8c..6d0989a 100644
@@ -31,7 +31,6 @@
  #include "report-maker.h"
  #include "report-formatter-csv.h"
  #include "report-formatter-html.h"
-#include "report-formatter-null.h"
 
  /* ************************************************************************ */
 

@@ -114,7 +113,7 @@ report_maker::setup_report_formatter()
  	else if (type == REPORT_CSV)
  		formatter = new report_formatter_csv();
  	else if (type == REPORT_OFF)
-		formatter = new report_formatter_null();
+		formatter = new basic_report_formatter();
  	else
  		assert(false);
  }

src/report/report-maker.h +2 -2

index 75e0d06..df8d20c 100644
@@ -162,7 +162,7 @@ enum cell_type {
 
  /* ************************************************************************ */
 
-class report_formatter;
+class basic_report_formatter;
 
  class report_maker
  {

@@ -202,7 +202,7 @@ private:
  	void end_paragraph();
 
  	report_type type;
-	report_formatter *formatter;
+	basic_report_formatter *formatter;
  	bool cell_opened, row_opened, table_opened, section_opened,
  	     paragraph_opened;
  };

_______________________________________________
PowerTop mailing list
PowerTop(a)lists.01.org
https://lists.01.org/mailman/listinfo/powertop

---
  src/Makefile.am                      |   1 -
  src/report/basic-report-formatter.h  |  65 +++++++++++++
  src/report/report-formatter-base.cpp |  14 +--
  src/report/report-formatter-base.h   |   4 +-
  src/report/report-formatter-csv.cpp  |  42 --------
  src/report/report-formatter-csv.h    |  10 +-
  src/report/report-formatter-html.h   |   2 +-
  src/report/report-formatter-null.cpp | 179 -----------------------------------
  src/report/report-formatter-null.h   |  66 -------------
  src/report/report-formatter.h        |  65 -------------
  src/report/report-maker.cpp          |   3 +-
  src/report/report-maker.h            |   4 +-
  12 files changed, 79 insertions(+), 376 deletions(-)

src/report/basic-report-formatter.h new file +16 -0

+	virtual void clear_result() {};
+
+	virtual void add(const char *str) {};
+	virtual void addv(const char *fmt, va_list ap) {};
+
+	virtual void add_header(const char *header, int level) {};
+
+	virtual void begin_section(section_type stype) {};
+	virtual void end_section() {};
+
+	virtual void begin_table(table_type ttype) {};
+	virtual void end_table() {};
+
+	virtual void begin_row(row_type rtype) {};
+	virtual void end_row() {};
+

He Sergey,

Sergey Senozhatsky wrote:

1)
- "Basic report_formatter::get_resul call\n"
+ "basic_report_formatter::get_result call\n"

src/report/basic-report-formatter.h line 63

Quoted 5 lines

src/report/basic-report-formatter.h new file +5 -0

+	virtual void end_paragraph() {};
+
+	/* For quad-colouring CPU tables in HTML */
+	virtual void set_cpu_number(int nr) {};
+};

2) You don't need semicolon after closing bracket in class member inline definition in C++.