Re: [Powertop] [PATCH 07/12] move options structure to function scope

Re: [Powertop] [PATCH 07/12] move options structure to function scope

[Alexandra Yates]

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


> On (08/03/14 10:34), Sami Kerola wrote:
>> On 3 August 2014 04:52, Sergey Senozhatsky
>> <sergey.senozhatsky(a)gmail.com> wrote:
>> > On (08/02/14 23:20), Sami Kerola wrote:
>> >> On 2 August 2014 14:47, Sergey Senozhatsky
>> <sergey.senozhatsky(a)gmail.com> wrote:
>> >> >> Subject: [Powertop] [PATCH 07/12] move options structure to
>> function scope
>> >> >
>> >> > what for?
>> >>
>> >> They've told me keeping variables in as narrow scope as possible is a
>> >> virtue. Even if it would not be I don't see any harm of moving
>> >> everything out of global scope when ever possible.
>> >>
>> >> https://www.securecoding.cert.org/confluence/display/cplusplus/DCL07-CPP.+Minimize+the+scope+of+variables+and+methods
>> >
>> > this is different. let's keep it as is.
>>
>> Hi Sergey,
>>
>> That's alright the scope move is reverted. When I did that I started
>> to look the option structure, and later switch case segment. I started
>> to wonder why '-a' is marked as an option in structure but it is
>> missing from short options in optstring. That lead me to write two
>> small clean ups.
>>
>> https://github.com/kerolasa/powertop/commit/09a3bb68d1f92c30675679b64d63cddeafbfbce7
>> https://github.com/kerolasa/powertop/commit/8d81534bf55486b5230e18b217524a07694c5e9e
>>
>> Reminder. As mentioned yesterday, the patch series in maillist is
>> broken. Up to date versions of these changes are in my github branch,
>> which makes this a pull request.
>>
>>   https://github.com/kerolasa/powertop sami
>>
>
> Hi,
>
> powertop does not pull from outside repos. please resend
> your patch series to the list. thanks for your contribution.
>
> 	-ss
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
>

Hello All,

Welcome Sami to PowerTOP, and thank you very much for submitting your
patches, and everyone else for reviewing them.

Sami, I would appreciate you make the changes people requested on the
list, and be prepared for further rounds of reviews/changes of your
patches.  This is the current process PowerTOP developers do code reviews.

Here are few other things to consider before sending your patches for
PowerTOP:

As I mentioned on a previous thread PowerTOp uses the linux kernel coding
style, please change all your code to follow this guidelines.
https://www.kernel.org/doc/Documentation/CodingStyle.

Before creating a set of patches please check the code using 
https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl

Lastly, when creating your new set of patches use the flag
--subject-prefix="PATCH Vx" x being the revision version.  That will make
things easy on the mailing list.

After your code gets an ok from the community I will go ahead and add it
to mainline.

Thank you very much again for sending all your changes,
Alexandra.

Re: [Powertop] [PATCH 07/12] move options structure to function scope

[Sami Kerola]

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

On 5 August 2014 00:41, Alexandra Yates <alexandra.yates(a)linux.intel.com> wrote:

Hi Alexandra,

> Welcome Sami to PowerTOP, and thank you very much for submitting your
> patches, and everyone else for reviewing them.

Thank you indeed. It's always nice feedback, and critical eyes looking
my changes
that I've found to be way too often to be less than perfect.

> Sami, I would appreciate you make the changes people requested on the
> list, and be prepared for further rounds of reviews/changes of your
> patches.  This is the current process PowerTOP developers do code reviews.

AFAIK all changes that were proposed, and work, are done. The backslash
removal for instance did not happen, primarily because it would break builds.

> Here are few other things to consider before sending your patches for
> PowerTOP:
>
> As I mentioned on a previous thread PowerTOp uses the linux kernel coding
> style, please change all your code to follow this guidelines.
> https://www.kernel.org/doc/Documentation/CodingStyle.

Ok.

> Before creating a set of patches please check the code using
> https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl

Done. In total 0 errors, 0 warnings.

> Lastly, when creating your new set of patches use the flag
> --subject-prefix="PATCH Vx" x being the revision version.  That will make
> things easy on the mailing list.

Are you sure? That makes the subject lines to look following.

Subject: [PATCH Vx 01/14] configure: use vertical lists

I think I need to break the rules and drop the ' Vx' when resubmitting
the change set.

> After your code gets an ok from the community I will go ahead and add it
> to mainline.

Ack.

> Thank you very much again for sending all your changes,

NP. All I initially wanted to do was to get rid of the ncurses
blinking when --auto-tuning, but somehow the other changes just
started to happen. Hopefully that was good thing.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [Powertop] [PATCH 07/12] move options structure to function scope

[Sergey Senozhatsky]

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

On (08/04/14 16:41), Alexandra Yates wrote:
> > On (08/03/14 10:34), Sami Kerola wrote:
> >> On 3 August 2014 04:52, Sergey Senozhatsky
> >> <sergey.senozhatsky(a)gmail.com> wrote:
> >> > On (08/02/14 23:20), Sami Kerola wrote:
> >> >> On 2 August 2014 14:47, Sergey Senozhatsky
> >> <sergey.senozhatsky(a)gmail.com> wrote:
> >> >> >> Subject: [Powertop] [PATCH 07/12] move options structure to
> >> function scope
> >> >> >
> >> >> > what for?
> >> >>
> >> >> They've told me keeping variables in as narrow scope as possible is a
> >> >> virtue. Even if it would not be I don't see any harm of moving
> >> >> everything out of global scope when ever possible.
> >> >>
> >> >> https://www.securecoding.cert.org/confluence/display/cplusplus/DCL07-CPP.+Minimize+the+scope+of+variables+and+methods
> >> >
> >> > this is different. let's keep it as is.
> >>
> >> Hi Sergey,
> >>
> >> That's alright the scope move is reverted. When I did that I started
> >> to look the option structure, and later switch case segment. I started
> >> to wonder why '-a' is marked as an option in structure but it is
> >> missing from short options in optstring. That lead me to write two
> >> small clean ups.
> >>
> >> https://github.com/kerolasa/powertop/commit/09a3bb68d1f92c30675679b64d63cddeafbfbce7
> >> https://github.com/kerolasa/powertop/commit/8d81534bf55486b5230e18b217524a07694c5e9e
> >>
> >> Reminder. As mentioned yesterday, the patch series in maillist is
> >> broken. Up to date versions of these changes are in my github branch,
> >> which makes this a pull request.
> >>
> >>   https://github.com/kerolasa/powertop sami
> >>
> >
> > Hi,
> >
> > powertop does not pull from outside repos. please resend
> > your patch series to the list. thanks for your contribution.
> >
> > 	-ss
> 
> Hello All,
> 
> Welcome Sami to PowerTOP, and thank you very much for submitting your
> patches, and everyone else for reviewing them.
> 
> Sami, I would appreciate you make the changes people requested on the
> list, and be prepared for further rounds of reviews/changes of your
> patches.  This is the current process PowerTOP developers do code reviews.
> 
> Here are few other things to consider before sending your patches for
> PowerTOP:
> 
> As I mentioned on a previous thread PowerTOp uses the linux kernel coding
> style, please change all your code to follow this guidelines.
> https://www.kernel.org/doc/Documentation/CodingStyle.
> 
> Before creating a set of patches please check the code using 
> https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl

I doubt that kernel's checkpatch speaks C++, especially STL (iow, templates).

	-ss

> Lastly, when creating your new set of patches use the flag
> --subject-prefix="PATCH Vx" x being the revision version.  That will make
> things easy on the mailing list.
> 
> After your code gets an ok from the community I will go ahead and add it
> to mainline.
> 
> Thank you very much again for sending all your changes,
> Alexandra.
> 

Re: [Powertop] [PATCH 07/12] move options structure to function scope

[Sergey Senozhatsky]

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

On (08/03/14 10:34), Sami Kerola wrote:
> On 3 August 2014 04:52, Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com> wrote:
> > On (08/02/14 23:20), Sami Kerola wrote:
> >> On 2 August 2014 14:47, Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com> wrote:
> >> >> Subject: [Powertop] [PATCH 07/12] move options structure to function scope
> >> >
> >> > what for?
> >>
> >> They've told me keeping variables in as narrow scope as possible is a
> >> virtue. Even if it would not be I don't see any harm of moving
> >> everything out of global scope when ever possible.
> >>
> >> https://www.securecoding.cert.org/confluence/display/cplusplus/DCL07-CPP.+Minimize+the+scope+of+variables+and+methods
> >
> > this is different. let's keep it as is.
> 
> Hi Sergey,
> 
> That's alright the scope move is reverted. When I did that I started
> to look the option structure, and later switch case segment. I started
> to wonder why '-a' is marked as an option in structure but it is
> missing from short options in optstring. That lead me to write two
> small clean ups.
> 
> https://github.com/kerolasa/powertop/commit/09a3bb68d1f92c30675679b64d63cddeafbfbce7
> https://github.com/kerolasa/powertop/commit/8d81534bf55486b5230e18b217524a07694c5e9e
> 
> Reminder. As mentioned yesterday, the patch series in maillist is
> broken. Up to date versions of these changes are in my github branch,
> which makes this a pull request.
> 
>   https://github.com/kerolasa/powertop sami
>

Hi,

powertop does not pull from outside repos. please resend
your patch series to the list. thanks for your contribution.

	-ss

Re: [Powertop] [PATCH 07/12] move options structure to function scope

[Sami Kerola]

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

On 3 August 2014 04:52, Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com> wrote:
> On (08/02/14 23:20), Sami Kerola wrote:
>> On 2 August 2014 14:47, Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com> wrote:
>> >> Subject: [Powertop] [PATCH 07/12] move options structure to function scope
>> >
>> > what for?
>>
>> They've told me keeping variables in as narrow scope as possible is a
>> virtue. Even if it would not be I don't see any harm of moving
>> everything out of global scope when ever possible.
>>
>> https://www.securecoding.cert.org/confluence/display/cplusplus/DCL07-CPP.+Minimize+the+scope+of+variables+and+methods
>
> this is different. let's keep it as is.

Hi Sergey,

That's alright the scope move is reverted. When I did that I started
to look the option structure, and later switch case segment. I started
to wonder why '-a' is marked as an option in structure but it is
missing from short options in optstring. That lead me to write two
small clean ups.

https://github.com/kerolasa/powertop/commit/09a3bb68d1f92c30675679b64d63cddeafbfbce7
https://github.com/kerolasa/powertop/commit/8d81534bf55486b5230e18b217524a07694c5e9e

Reminder. As mentioned yesterday, the patch series in maillist is
broken. Up to date versions of these changes are in my github branch,
which makes this a pull request.

  https://github.com/kerolasa/powertop sami

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [Powertop] [PATCH 07/12] move options structure to function scope

[Sergey Senozhatsky]

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

Hello,

On (08/02/14 23:20), Sami Kerola wrote:
> On 2 August 2014 14:47, Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com> wrote:
> >> Subject: [Powertop] [PATCH 07/12] move options structure to function scope
> >
> > what for?
> 
> They've told me keeping variables in as narrow scope as possible is a
> virtue. Even if it would not be I don't see any harm of moving
> everything out of global scope when ever possible.
> 
> https://www.securecoding.cert.org/confluence/display/cplusplus/DCL07-CPP.+Minimize+the+scope+of+variables+and+methods

this is different. let's keep it as is.

	-ss

Re: [Powertop] [PATCH 07/12] move options structure to function scope

[Sami Kerola]

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

On 2 August 2014 14:47, Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com> wrote:
>> Subject: [Powertop] [PATCH 07/12] move options structure to function scope
>
> what for?

They've told me keeping variables in as narrow scope as possible is a
virtue. Even if it would not be I don't see any harm of moving
everything out of global scope when ever possible.

https://www.securecoding.cert.org/confluence/display/cplusplus/DCL07-CPP.+Minimize+the+scope+of+variables+and+methods

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [Powertop] [PATCH 07/12] move options structure to function scope

[Sergey Senozhatsky]

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

On (08/02/14 12:54), Sami Kerola wrote:
> Date: Sat,  2 Aug 2014 12:54:12 +0100
> From: Sami Kerola <kerolasa(a)iki.fi>
> To: powertop(a)lists.01.org
> Subject: [Powertop] [PATCH 07/12] move options structure to function scope
> X-Mailer: git-send-email 2.0.3
> 
> Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
> ---
>  src/main.cpp | 35 ++++++++++++++++++-----------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/src/main.cpp b/src/main.cpp
> index 99a985f..910fe5c 100644
> --- a/src/main.cpp
> +++ b/src/main.cpp
> @@ -69,23 +69,6 @@ int debug_learning = 0;
>  unsigned time_out = 20;
>  int leave_powertop = 0;
>  
> -static const struct option long_options[] =
> -{
> -	/* These options set a flag. */
> -	{"debug", no_argument, &debug_learning, 'd'},
> -	{"version", no_argument, NULL, 'V'},
> -	{"help",no_argument, NULL, 'u'}, /* u for usage */
> -	{"calibrate",no_argument, NULL, 'c'},
> -	{"auto-tune",no_argument, NULL, 'a'},
> -	{"html", optional_argument, NULL, 'h'},
> -	{"csv", optional_argument, NULL, 'C'},
> -	{"extech", optional_argument, NULL, 'e'},
> -	{"time", optional_argument, NULL, 't'},
> -	{"iteration", optional_argument, NULL, 'i'},
> -	{"workload", optional_argument, NULL, 'w'},
> -	{"quiet", no_argument, NULL, 'q'},
> -	{NULL, 0, NULL, 0}
> -};
>  
>  static void print_version()
>  {
> @@ -370,6 +353,24 @@ int main(int argc, char **argv)
>  	char workload[4096] = {0,};
>  	int  iterations = 1, auto_tune = 0;
>  
> +	static const struct option long_options[] =
> +	{
> +		/* These options set a flag. */
> +		{"debug", no_argument, &debug_learning, 'd'},
> +		{"version", no_argument, NULL, 'V'},
> +		{"help",no_argument, NULL, 'u'}, /* u for usage */
> +		{"calibrate",no_argument, NULL, 'c'},
> +		{"auto-tune",no_argument, NULL, 'a'},
> +		{"html", optional_argument, NULL, 'h'},
> +		{"csv", optional_argument, NULL, 'C'},
> +		{"extech", optional_argument, NULL, 'e'},
> +		{"time", optional_argument, NULL, 't'},
> +		{"iteration", optional_argument, NULL, 'i'},
> +		{"workload", optional_argument, NULL, 'w'},
> +		{"quiet", no_argument, NULL, 'q'},
> +		{NULL, 0, NULL, 0}
> +	};

what for?

	-ss

>  	set_new_handler(out_of_memory);
>  
>  	setlocale (LC_ALL, "");
> -- 
> 2.0.3
> 
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
> 

[Powertop] [PATCH 07/12] move options structure to function scope

[Sami Kerola]

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

Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
 src/main.cpp | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/src/main.cpp b/src/main.cpp
index 99a985f..910fe5c 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -69,23 +69,6 @@ int debug_learning = 0;
 unsigned time_out = 20;
 int leave_powertop = 0;
 
-static const struct option long_options[] =
-{
-	/* These options set a flag. */
-	{"debug", no_argument, &debug_learning, 'd'},
-	{"version", no_argument, NULL, 'V'},
-	{"help",no_argument, NULL, 'u'}, /* u for usage */
-	{"calibrate",no_argument, NULL, 'c'},
-	{"auto-tune",no_argument, NULL, 'a'},
-	{"html", optional_argument, NULL, 'h'},
-	{"csv", optional_argument, NULL, 'C'},
-	{"extech", optional_argument, NULL, 'e'},
-	{"time", optional_argument, NULL, 't'},
-	{"iteration", optional_argument, NULL, 'i'},
-	{"workload", optional_argument, NULL, 'w'},
-	{"quiet", no_argument, NULL, 'q'},
-	{NULL, 0, NULL, 0}
-};
 
 static void print_version()
 {
@@ -370,6 +353,24 @@ int main(int argc, char **argv)
 	char workload[4096] = {0,};
 	int  iterations = 1, auto_tune = 0;
 
+	static const struct option long_options[] =
+	{
+		/* These options set a flag. */
+		{"debug", no_argument, &debug_learning, 'd'},
+		{"version", no_argument, NULL, 'V'},
+		{"help",no_argument, NULL, 'u'}, /* u for usage */
+		{"calibrate",no_argument, NULL, 'c'},
+		{"auto-tune",no_argument, NULL, 'a'},
+		{"html", optional_argument, NULL, 'h'},
+		{"csv", optional_argument, NULL, 'C'},
+		{"extech", optional_argument, NULL, 'e'},
+		{"time", optional_argument, NULL, 't'},
+		{"iteration", optional_argument, NULL, 'i'},
+		{"workload", optional_argument, NULL, 'w'},
+		{"quiet", no_argument, NULL, 'q'},
+		{NULL, 0, NULL, 0}
+	};
+
 	set_new_handler(out_of_memory);
 
 	setlocale (LC_ALL, "");
-- 
2.0.3