[Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Igor Zhbanov]

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

Get rid of TRUE and FALSE ncurses' defines used for boolean variables
initialization. This fix also helps build without ncurses support.
---
 src/main.cpp          |    6 +++---
 src/tuning/tuning.cpp |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/main.cpp b/src/main.cpp
index 7a1b976..9f53b46 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -354,7 +354,7 @@ int main(int argc, char **argv)
 {
 	int option_index;
 	int c;
-	bool wantreport = FALSE;
+	bool wantreport = false;
 	char filename[4096];
 	char workload[4096] = {0,};
 	int  iterations = 1;
@@ -396,7 +396,7 @@ int main(int argc, char **argv)
 				break;
 
 			case 'h': /* html report */
-				wantreport = TRUE;
+				wantreport = true;
 				reporttype = 1;
 				sprintf(filename, "%s", optarg ? optarg : "PowerTOP.html" );
 				break;
@@ -417,7 +417,7 @@ int main(int argc, char **argv)
 				break;
 				
 			case 'C': /* csv report*/
-				wantreport = TRUE;
+				wantreport = true;
 				reporttype = 0;
 				sprintf(filename, "%s", optarg ? optarg : "PowerTOP.csv");
 				break;
diff --git a/src/tuning/tuning.cpp b/src/tuning/tuning.cpp
index 1a90417..865d986 100644
--- a/src/tuning/tuning.cpp
+++ b/src/tuning/tuning.cpp
@@ -43,7 +43,7 @@
 #include "../lib.h"
 
 static void sort_tunables(void);
-static bool should_clear = FALSE;
+static bool should_clear = false;
 
 #ifndef DISABLE_NCURSES
 class tuning_window: public tab_window {
@@ -103,7 +103,7 @@ static void __tuning_update_display(int cursor_pos)
 		return;
 
 	if (should_clear) {
-		should_clear = FALSE;
+		should_clear = false;
 		wclear(win);
 	}
 
@@ -185,7 +185,7 @@ static bool tunables_sort(class tunable * i, class tunable * j)
 void tuning_window::window_refresh()
 {
 	clear_tuning();
-	should_clear = TRUE;
+	should_clear = true;
 	init_tuning();
 }
 
-- 
1.7.5.4

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Chris Ferron]

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

On 07/23/2012 06:23 AM, Arjan van de Ven wrote:
> On 7/23/2012 12:31 AM, Magnus Fromreide wrote:
>
>>> Without it PowerTOP will not know what system call number
>>> __NR_perf_event_open has on ARM.
>> Why can't it take __NR_perf_event_open from the libc headers?
>> That is, why can't the whole ifdef under /* some people have stale
>> headers */ go?
>>
>> Powertop is rather demanding with regards to the environment anyhow and
>> magic constants are never fun.
> 100% agreed; I briefly talked to Chris on Friday based on this patch and
> that is the conclusion we came to as well.
This fix went in Friday as well. We should be now taking 
__NR_perf_event_open from sys/syscall.h now.

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

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Arjan van de Ven]

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

On 7/23/2012 12:31 AM, Magnus Fromreide wrote:

>> Without it PowerTOP will not know what system call number 
>> __NR_perf_event_open has on ARM.
> 
> Why can't it take __NR_perf_event_open from the libc headers?
> That is, why can't the whole ifdef under /* some people have stale
> headers */ go?
> 
> Powertop is rather demanding with regards to the environment anyhow and
> magic constants are never fun.

100% agreed; I briefly talked to Chris on Friday based on this patch and
that is the conclusion we came to as well.

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Igor Zhbanov]

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

Hi Magnus,

Magnus Fromreide wrote:
> On Mon, 2012-07-23 at 10:32 +0400, Igor Zhbanov wrote:
>> Hi Chris!
>> Thank you for your answer.
>>
>> Chris Ferron wrote:
>>> On 07/20/2012 12:06 AM, Igor Zhbanov wrote:
>>>> Linaro has made only few commits after forking from mainline.
>>>>
>>>> They are:
>>>>
>>>> 1) adding __NR_perf_event_open define
>>> Please insure this is fresh code authored by you and detailed if you
>>> submit, and I will be happy to take it.
>> Yes, this is just a system call number copied from unistd.h for ARM
>> platform.
>> Without it PowerTOP will not know what system call number
>> __NR_perf_event_open has on ARM.
> Why can't it take __NR_perf_event_open from the libc headers?
> That is, why can't the whole ifdef under /* some people have stale
> headers */ go?
>
> Powertop is rather demanding with regards to the environment anyhow and
> magic constants are never fun.
>
> /MF
I can confirm that PowerTOP successfully builds for ARM both with ScratchBox and with a cross-compiler without that define. I send this patch to be uniform with other platforms as I can't guarantee that all platforms has correct headers.

Perhaps that defines can be removed.

-- 
Best regards,
Igor Zhbanov,
Expert Software Engineer,
phone: +7 (495) 797 25 00 ext 3806
e-mail: i.zhbanov(a)samsung.com

ASWG, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Magnus Fromreide]

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

On Mon, 2012-07-23 at 10:32 +0400, Igor Zhbanov wrote:
> Hi Chris!
> Thank you for your answer.
> 
> Chris Ferron wrote:
> > On 07/20/2012 12:06 AM, Igor Zhbanov wrote:
> >>
> >> Linaro has made only few commits after forking from mainline.
> >>
> >> They are:
> >>
> >> 1) adding __NR_perf_event_open define
> > Please insure this is fresh code authored by you and detailed if you 
> > submit, and I will be happy to take it.
> Yes, this is just a system call number copied from unistd.h for ARM 
> platform.
> Without it PowerTOP will not know what system call number 
> __NR_perf_event_open has on ARM.

Why can't it take __NR_perf_event_open from the libc headers?
That is, why can't the whole ifdef under /* some people have stale
headers */ go?

Powertop is rather demanding with regards to the environment anyhow and
magic constants are never fun.

/MF

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Igor Zhbanov]

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

Hi Chris!
Thank you for your answer.

Chris Ferron wrote:
> On 07/20/2012 12:06 AM, Igor Zhbanov wrote:
>> Anyway, that's all major Linaro's changes.
> I understand you don't see the severity of the issue. But I must 
> insist that no more third party submittal.
>> There are small minor changes to Android.mk, but it is not suitable 
>> for mainline.
> Please insure this is fresh code authored by you if you submit, and I 
> will be happy to take it.
No, that code is not mine. I even doesn't build PowerTOP for Android.
>>
>> Linaro has made only few commits after forking from mainline.
>>
>> They are:
>>
>> 1) adding __NR_perf_event_open define
> Please insure this is fresh code authored by you and detailed if you 
> submit, and I will be happy to take it.
Yes, this is just a system call number copied from unistd.h for ARM 
platform.
Without it PowerTOP will not know what system call number 
__NR_perf_event_open has on ARM.
>>
>> 2) building without libpci
> Done
>> 3) wrong fix for hyper-threading cpu
> Not needed
>> 4) set default variable for TERM and TERMINFO variables
> Please insure this is fresh code authored by you if you submit, and I 
> will be happy to take it.
I don't need it for my builds. It was from Linaro's commits. I don't 
even send it to list.
>>
>> 5) some changes in Android.mk
> See above
Same as above.
>> 6) change boardname code.
> I will have a fix for this today 7/20/12 PST
Same as above.
>>
>> That's all. So the git trees in the fact almost the same.
>>
>> And now for successful build for ARM devices only two things left:
>>
>> 1) Define __NR_perf_event_open for ARM
>> 2) Change csstoh build rules so that will be built with a cross-compiler
>> or replace it with a shell script.
> Shell Script will not be considered, other solutions will be considered.
I saw suggestion to use HOSTCC variable to compile csstoh.c for build 
platform
but I don't know automake good enough to modify it so, that after 
running autotools
rules for building csstoh will contain HOSTCC instead of CC. I don't know
how to combine HOSTCC and noinst.

If you now good guide for writing automakes for building with a 
cross-compiler,
please tell me.

Once again, thank you for your response.

-- 
Best regards,
Igor Zhbanov,
Expert Software Engineer,
phone: +7 (495) 797 25 00 ext 3806
e-mail: i.zhbanov(a)samsung.com

ASWG, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Chris Ferron]

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

On 07/20/2012 12:06 AM, Igor Zhbanov wrote:
> Anyway, that's all major Linaro's changes.
I understand you don't see the severity of the issue. But I must insist 
that no more third party submittal.
> There are small minor changes to Android.mk, but it is not suitable 
> for mainline.
Please insure this is fresh code authored by you if you submit, and I 
will be happy to take it.
>
> Linaro has made only few commits after forking from mainline.
>
> They are:
>
> 1) adding __NR_perf_event_open define
Please insure this is fresh code authored by you and detailed if you 
submit, and I will be happy to take it.
>
> 2) building without libpci
Done
> 3) wrong fix for hyper-threading cpu
Not needed
> 4) set default variable for TERM and TERMINFO variables
Please insure this is fresh code authored by you if you submit, and I 
will be happy to take it.
>
> 5) some changes in Android.mk
See above
> 6) change boardname code.
I will have a fix for this today 7/20/12 PST
>
> That's all. So the git trees in the fact almost the same.
>
> And now for successful build for ARM devices only two things left:
>
> 1) Define __NR_perf_event_open for ARM
> 2) Change csstoh build rules so that will be built with a cross-compiler
> or replace it with a shell script.
Shell Script will not be considered, other solutions will be considered.
>
> Arjan van de Ven wrote:
>>>      Please do not merge Intel's and Linaro repositories and send 
>>> patches
>>> for what comes of it.
>>> If the Linaro maintainers want to do an upstream merge or if
>>> contributors of Linaro want to upstream
>>> patches they have authored then great.
>>> Submitting patches based on other peoples work is not agreeable to 
>>> me. I
>>> will not take patches that
>>> I even suspect isn't from the original authors, and or without the
>>> authors consent. Do not just merge code bases
>>> and generate patches based on the diff, and send it to the list. They
>>> will not be taken.
>> specifically, I really really insist on keeping the correct authorship
>> information ("Author" git tag etc) for all the code going into PowerTOP.
>> Credit where credit is due, but also for tracking the origin and
>> copyright holders of the code.

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Igor Zhbanov]

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

Anyway, that's all major Linaro's changes.
There are small minor changes to Android.mk, but it is not suitable for 
mainline.

Linaro has made only few commits after forking from mainline.

They are:

1) adding __NR_perf_event_open define
2) building without libpci
3) wrong fix for hyper-threading cpu
4) set default variable for TERM and TERMINFO variables
5) some changes in Android.mk
6) change boardname code.

That's all. So the git trees in the fact almost the same.

And now for successful build for ARM devices only two things left:

1) Define __NR_perf_event_open for ARM
2) Change csstoh build rules so that will be built with a cross-compiler
or replace it with a shell script.

Arjan van de Ven wrote:
>>      Please do not merge Intel's and Linaro repositories and send patches
>> for what comes of it.
>> If the Linaro maintainers want to do an upstream merge or if
>> contributors of Linaro want to upstream
>> patches they have authored then great.
>> Submitting patches based on other peoples work is not agreeable to me. I
>> will not take patches that
>> I even suspect isn't from the original authors, and or without the
>> authors consent. Do not just merge code bases
>> and generate patches based on the diff, and send it to the list. They
>> will not be taken.
> specifically, I really really insist on keeping the correct authorship
> information ("Author" git tag etc) for all the code going into PowerTOP.
> Credit where credit is due, but also for tracking the origin and
> copyright holders of the code.
-- 
Best regards,
Igor Zhbanov,
Expert Software Engineer,
phone: +7 (495) 797 25 00 ext 3806
e-mail: i.zhbanov(a)samsung.com

ASWG, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Arjan van de Ven]

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


>     Please do not merge Intel's and Linaro repositories and send patches
> for what comes of it.
> If the Linaro maintainers want to do an upstream merge or if
> contributors of Linaro want to upstream
> patches they have authored then great.
> Submitting patches based on other peoples work is not agreeable to me. I
> will not take patches that
> I even suspect isn't from the original authors, and or without the
> authors consent. Do not just merge code bases
> and generate patches based on the diff, and send it to the list. They
> will not be taken.
> 


specifically, I really really insist on keeping the correct authorship
information ("Author" git tag etc) for all the code going into PowerTOP.
Credit where credit is due, but also for tracking the origin and
copyright holders of the code.

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Chris Ferron]

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

On 07/18/2012 06:21 AM, Igor Zhbanov wrote:
> Arjan van de Ven wrote:
>> On 7/18/2012 2:17 AM, Igor Zhbanov wrote:
>>> Get rid of TRUE and FALSE ncurses' defines used for boolean variables
>>> initialization. This fix also helps build without ncurses support.
>> while I appreciate going to C++ true/false, building without ncurses
>> isn't really a goal.
>> (even on android, building ncurses is easy, and it does make the tool SO
>> much more useful)
> It is not good that C++ code depends on TRUE/FALSE defines
> (defined in ncurses.h) in functions that are not related to ncurses.
>
> P.S. I have merged Intel's and Linaro's repositories and will send 
> patches for
> review soon. I don't know why but Linaro wants to build PowerTOP 
> without ncurses.
> So I will send a patch that makes ncurses and libpci dependencies 
> conditional.
>

     Please do not merge Intel's and Linaro repositories and send 
patches for what comes of it.
If the Linaro maintainers want to do an upstream merge or if 
contributors of Linaro want to upstream
patches they have authored then great.
Submitting patches based on other peoples work is not agreeable to me. I 
will not take patches that
I even suspect isn't from the original authors, and or without the 
authors consent. Do not just merge code bases
and generate patches based on the diff, and send it to the list. They 
will not be taken.

-Chris Ferron

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Chris Ferron]

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

On 07/18/2012 02:17 AM, Igor Zhbanov wrote:
> Get rid of TRUE and FALSE ncurses' defines used for boolean variables
> initialization. This fix also helps build without ncurses support.
> ---
>   src/main.cpp          |    6 +++---
>   src/tuning/tuning.cpp |    6 +++---
>   2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/main.cpp b/src/main.cpp
> index 7a1b976..9f53b46 100644
> --- a/src/main.cpp
> +++ b/src/main.cpp
> @@ -354,7 +354,7 @@ int main(int argc, char **argv)
>   {
>   	int option_index;
>   	int c;
> -	bool wantreport = FALSE;
> +	bool wantreport = false;
>   	char filename[4096];
>   	char workload[4096] = {0,};
>   	int  iterations = 1;
> @@ -396,7 +396,7 @@ int main(int argc, char **argv)
>   				break;
>   
>   			case 'h': /* html report */
> -				wantreport = TRUE;
> +				wantreport = true;
>   				reporttype = 1;
>   				sprintf(filename, "%s", optarg ? optarg : "PowerTOP.html" );
>   				break;
> @@ -417,7 +417,7 @@ int main(int argc, char **argv)
>   				break;
>   				
>   			case 'C': /* csv report*/
> -				wantreport = TRUE;
> +				wantreport = true;
>   				reporttype = 0;
>   				sprintf(filename, "%s", optarg ? optarg : "PowerTOP.csv");
>   				break;
> diff --git a/src/tuning/tuning.cpp b/src/tuning/tuning.cpp
> index 1a90417..865d986 100644
> --- a/src/tuning/tuning.cpp
> +++ b/src/tuning/tuning.cpp
> @@ -43,7 +43,7 @@
>   #include "../lib.h"
>   
>   static void sort_tunables(void);
> -static bool should_clear = FALSE;
> +static bool should_clear = false;
>   
>   #ifndef DISABLE_NCURSES
>   class tuning_window: public tab_window {
> @@ -103,7 +103,7 @@ static void __tuning_update_display(int cursor_pos)
>   		return;
>   
>   	if (should_clear) {
> -		should_clear = FALSE;
> +		should_clear = false;
>   		wclear(win);
>   	}
>   
> @@ -185,7 +185,7 @@ static bool tunables_sort(class tunable * i, class tunable * j)
>   void tuning_window::window_refresh()
>   {
>   	clear_tuning();
> -	should_clear = TRUE;
> +	should_clear = true;
>   	init_tuning();
>   }
>   
Your patch has been merged
Thank You

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Igor Zhbanov]

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

Arjan van de Ven wrote:
> On 7/18/2012 2:17 AM, Igor Zhbanov wrote:
>> Get rid of TRUE and FALSE ncurses' defines used for boolean variables
>> initialization. This fix also helps build without ncurses support.
> while I appreciate going to C++ true/false, building without ncurses
> isn't really a goal.
> (even on android, building ncurses is easy, and it does make the tool SO
> much more useful)
It is not good that C++ code depends on TRUE/FALSE defines
(defined in ncurses.h) in functions that are not related to ncurses.

P.S. I have merged Intel's and Linaro's repositories and will send patches for
review soon. I don't know why but Linaro wants to build PowerTOP without ncurses.
So I will send a patch that makes ncurses and libpci dependencies conditional.

-- 
Best regards,
Igor Zhbanov,
Expert Software Engineer,
phone: +7 (495) 797 25 00 ext 3806
e-mail: i.zhbanov(a)samsung.com

ASWG, Moscow R&D center, Samsung Electronics
12 Dvintsev street, building 1
127018, Moscow, Russian Federation

Re: [Powertop] [PATCH] Get rid of ncurses' TRUE and FALSE

[Arjan van de Ven]

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

On 7/18/2012 2:17 AM, Igor Zhbanov wrote:
> Get rid of TRUE and FALSE ncurses' defines used for boolean variables
> initialization. This fix also helps build without ncurses support.


while I appreciate going to C++ true/false, building without ncurses
isn't really a goal.
(even on android, building ncurses is easy, and it does make the tool SO
much more useful)