[PATCH 0/5] Trivial warning cleanups

[PATCH 0/5] Trivial warning cleanups

[Felipe Contreras]

Hi,

This patch series fixes all the warnings I get when building a kernel for my
laptop. Plus a few whitespace cleanups I found along the way.

Felipe Contreras (5):
  Trivial whitespace cleanups
  kbuild: fix warning when domainname is not available
  kbuild: mkcompile_h: trivial cleanups
  acpi: fix trivial warning
  acpi: fix trivial warnings caused by previous commmit

 arch/x86/include/asm/string_32.h |    1 -
 arch/x86/kernel/tsc.c            |    2 +-
 drivers/acpi/acpica/exfldio.c    |    8 ++++----
 drivers/acpi/acpica/tbfadt.c     |    1 -
 include/acpi/actypes.h           |    4 ++--
 scripts/mkcompile_h              |   12 +++++++++---
 6 files changed, 16 insertions(+), 12 deletions(-)

[PATCH 1/5] Trivial whitespace cleanups

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 arch/x86/include/asm/string_32.h |    1 -
 arch/x86/kernel/tsc.c            |    2 +-
 drivers/acpi/acpica/tbfadt.c     |    1 -
 3 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/string_32.h b/arch/x86/include/asm/string_32.h
index c86f452..ae907e6 100644
--- a/arch/x86/include/asm/string_32.h
+++ b/arch/x86/include/asm/string_32.h
@@ -65,7 +65,6 @@ static __always_inline void *__constant_memcpy(void *to, const void *from,
 	case 4:
 		*(int *)to = *(int *)from;
 		return to;
-
 	case 3:
 		*(short *)to = *(short *)from;
 		*((char *)to + 2) = *((char *)from + 2);
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 71f4368..8a2fc11 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -670,7 +670,7 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
 	if ((val == CPUFREQ_PRECHANGE  && freq->old < freq->new) ||
 			(val == CPUFREQ_POSTCHANGE && freq->old > freq->new) ||
 			(val == CPUFREQ_RESUMECHANGE)) {
-		*lpj = 	cpufreq_scale(loops_per_jiffy_ref, ref_freq, freq->new);
+		*lpj = cpufreq_scale(loops_per_jiffy_ref, ref_freq, freq->new);
 
 		tsc_khz = cpufreq_scale(tsc_khz_ref, ref_freq, freq->new);
 		if (!(freq->flags & CPUFREQ_CONST_LOOPS))
diff --git a/drivers/acpi/acpica/tbfadt.c b/drivers/acpi/acpica/tbfadt.c
index 82b02dc..c016335 100644
--- a/drivers/acpi/acpica/tbfadt.c
+++ b/drivers/acpi/acpica/tbfadt.c
@@ -275,7 +275,6 @@ void acpi_tb_parse_fadt(u32 table_index)
 
 void acpi_tb_create_local_fadt(struct acpi_table_header *table, u32 length)
 {
-
 	/*
 	 * Check if the FADT is larger than the largest table that we expect
 	 * (the ACPI 2.0/3.0 version). If so, truncate the table, and issue
-- 
1.6.5.rc1

[PATCH 2/5] kbuild: fix warning when domainname is not available

[Felipe Contreras]

Otherwise we get:
"dnsdomainname: Unknown host"

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 scripts/mkcompile_h |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/scripts/mkcompile_h b/scripts/mkcompile_h
index 6a12dd9..2e09f4a 100755
--- a/scripts/mkcompile_h
+++ b/scripts/mkcompile_h
@@ -66,9 +66,13 @@ UTS_TRUNCATE="sed -e s/\(.\{1,$UTS_LEN\}\).*/\1/"
   echo \#define LINUX_COMPILE_HOST \"`hostname | $UTS_TRUNCATE`\"
 
   if [ -x /bin/dnsdomainname ]; then
-    echo \#define LINUX_COMPILE_DOMAIN \"`dnsdomainname | $UTS_TRUNCATE`\"
+    domain=`dnsdomainname 2> /dev/null`
   elif [ -x /bin/domainname ]; then
-    echo \#define LINUX_COMPILE_DOMAIN \"`domainname | $UTS_TRUNCATE`\"
+    domain=`domainname 2> /dev/null`
+  fi
+
+  if $domain; then
+    echo \#define LINUX_COMPILE_DOMAIN \"`echo $domain | $UTS_TRUNCATE`\"
   else
     echo \#define LINUX_COMPILE_DOMAIN
   fi
-- 
1.6.5.rc1

[PATCH 3/5] kbuild: mkcompile_h: trivial cleanups

[Felipe Contreras]

UTS_TRUNCATTE is simpler this way, and now editors idetify this as a
shell script.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 scripts/mkcompile_h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/scripts/mkcompile_h b/scripts/mkcompile_h
index 2e09f4a..7ab9c52 100755
--- a/scripts/mkcompile_h
+++ b/scripts/mkcompile_h
@@ -1,3 +1,5 @@
+#!/bin/sh
+
 TARGET=$1
 ARCH=$2
 SMP=$3
@@ -50,7 +52,7 @@ UTS_VERSION="$UTS_VERSION $CONFIG_FLAGS $TIMESTAMP"
 # Truncate to maximum length
 
 UTS_LEN=64
-UTS_TRUNCATE="sed -e s/\(.\{1,$UTS_LEN\}\).*/\1/"
+UTS_TRUNCATE="cut -b -$UTS_LEN"
 
 # Generate a temporary compile.h
 
-- 
1.6.5.rc1

[PATCH 4/5] acpi: fix trivial warning

[Felipe Contreras]

drivers/acpi/acpica/tbfadt.c: In function ‘acpi_tb_create_local_fadt’:
arch/x86/include/asm/string_32.h:74: warning: array subscript is above array bounds

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 include/acpi/actypes.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index 37ba576..e312be7 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -438,8 +438,8 @@ typedef unsigned long long acpi_integer;
 
 #define ACPI_SET_BIT(target,bit)        ((target) |= (bit))
 #define ACPI_CLEAR_BIT(target,bit)      ((target) &= ~(bit))
-#define ACPI_MIN(a,b)                   (((a)<(b))?(a):(b))
-#define ACPI_MAX(a,b)                   (((a)>(b))?(a):(b))
+#define ACPI_MIN(a,b)                   min(a, b)
+#define ACPI_MAX(a,b)                   max(a, b)
 
 /* Size calculation */
 
-- 
1.6.5.rc1

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] acpi: fix trivial warning

[Felipe Contreras]

drivers/acpi/acpica/tbfadt.c: In function ‘acpi_tb_create_local_fadt’:
arch/x86/include/asm/string_32.h:74: warning: array subscript is above array bounds

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 include/acpi/actypes.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index 37ba576..e312be7 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -438,8 +438,8 @@ typedef unsigned long long acpi_integer;
 
 #define ACPI_SET_BIT(target,bit)        ((target) |= (bit))
 #define ACPI_CLEAR_BIT(target,bit)      ((target) &= ~(bit))
-#define ACPI_MIN(a,b)                   (((a)<(b))?(a):(b))
-#define ACPI_MAX(a,b)                   (((a)>(b))?(a):(b))
+#define ACPI_MIN(a,b)                   min(a, b)
+#define ACPI_MAX(a,b)                   max(a, b)
 
 /* Size calculation */
 
-- 
1.6.5.rc1

[PATCH 5/5] acpi: fix trivial warnings caused by previous commmit

[Felipe Contreras]

drivers/acpi/acpica/exfldio.c: In function ‘acpi_ex_extract_from_field’:
drivers/acpi/acpica/exfldio.c:761: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:761: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:761: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:780: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:780: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:780: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c: In function ‘acpi_ex_insert_into_field’:
drivers/acpi/acpica/exfldio.c:880: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:880: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:880: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:933: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:933: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:933: warning: comparison of distinct pointer types lacks a cast

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/acpi/acpica/exfldio.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpica/exfldio.c b/drivers/acpi/acpica/exfldio.c
index 6687be1..4affc5f 100644
--- a/drivers/acpi/acpica/exfldio.c
+++ b/drivers/acpi/acpica/exfldio.c
@@ -760,7 +760,7 @@ acpi_ex_extract_from_field(union acpi_operand_object *obj_desc,
 
 		ACPI_MEMCPY(((char *)buffer) + buffer_offset, &merged_datum,
 			    ACPI_MIN(obj_desc->common_field.access_byte_width,
-				     buffer_length - buffer_offset));
+				     (u8)(buffer_length - buffer_offset)));
 
 		buffer_offset += obj_desc->common_field.access_byte_width;
 		merged_datum =
@@ -779,7 +779,7 @@ acpi_ex_extract_from_field(union acpi_operand_object *obj_desc,
 
 	ACPI_MEMCPY(((char *)buffer) + buffer_offset, &merged_datum,
 		    ACPI_MIN(obj_desc->common_field.access_byte_width,
-			     buffer_length - buffer_offset));
+			     (u8)(buffer_length - buffer_offset)));
 
 	return_ACPI_STATUS(AE_OK);
 }
@@ -879,7 +879,7 @@ acpi_ex_insert_into_field(union acpi_operand_object *obj_desc,
 
 	ACPI_MEMCPY(&raw_datum, buffer,
 		    ACPI_MIN(obj_desc->common_field.access_byte_width,
-			     buffer_length - buffer_offset));
+			     (u8)(buffer_length - buffer_offset)));
 
 	merged_datum =
 	    raw_datum << obj_desc->common_field.start_field_bit_offset;
@@ -932,7 +932,7 @@ acpi_ex_insert_into_field(union acpi_operand_object *obj_desc,
 		buffer_offset += obj_desc->common_field.access_byte_width;
 		ACPI_MEMCPY(&raw_datum, ((char *)buffer) + buffer_offset,
 			    ACPI_MIN(obj_desc->common_field.access_byte_width,
-				     buffer_length - buffer_offset));
+				     (u8)(buffer_length - buffer_offset)));
 		merged_datum |=
 		    raw_datum << obj_desc->common_field.start_field_bit_offset;
 	}
-- 
1.6.5.rc1

Re: [PATCH 2/5] kbuild: fix warning when domainname is not available

[Mike Frysinger]

On Sun, Sep 13, 2009 at 15:38, Felipe Contreras wrote:
> +  if $domain; then

is this really correct ?  i think you meant to use:
[ -n "$domain" ]
-mike

Re: [PATCH 2/5] kbuild: fix warning when domainname is not available

[Felipe Contreras]

On Mon, Sep 14, 2009 at 12:12 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> On Sun, Sep 13, 2009 at 15:38, Felipe Contreras wrote:
>> +  if $domain; then
>
> is this really correct ?  i think you meant to use:
> [ -n "$domain" ]

What is the difference?

$domain unset
test -n "" -> false
test -> false

$domain is a valid string
test -n "string" -> true
test "string" -> true

The relevant rules are these:
* An omitted EXPRESSION defaults to false
* STRING equivalent to -n STRING

-- 
Felipe Contreras

Re: [PATCH 2/5] kbuild: fix warning when domainname is not available

[Mike Frysinger]

.
On Sun, Sep 13, 2009 at 17:42, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Mon, Sep 14, 2009 at 12:12 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
>> On Sun, Sep 13, 2009 at 15:38, Felipe Contreras wrote:
>>> +  if $domain; then
>>
>> is this really correct ?  i think you meant to use:
>> [ -n "$domain" ]
>
> What is the difference?
>
> $domain unset
> test -n "" -> false
> test -> false
>
> $domain is a valid string
> test -n "string" -> true
> test "string" -> true

except that you didnt invoke `test` anywhere.  you're executing the
contents of $domain.
-mike

Re: [PATCH 2/5] kbuild: fix warning when domainname is not available

[Felipe Contreras]

On Mon, Sep 14, 2009 at 12:58 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> .
> On Sun, Sep 13, 2009 at 17:42, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Mon, Sep 14, 2009 at 12:12 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
>>> On Sun, Sep 13, 2009 at 15:38, Felipe Contreras wrote:
>>>> +  if $domain; then
>>>
>>> is this really correct ?  i think you meant to use:
>>> [ -n "$domain" ]
>>
>> What is the difference?
>>
>> $domain unset
>> test -n "" -> false
>> test -> false
>>
>> $domain is a valid string
>> test -n "string" -> true
>> test "string" -> true
>
> except that you didnt invoke `test` anywhere.  you're executing the
> contents of $domain.

Ahh, I'll update it to:
[ "$domain" ]

-- 
Felipe Contreras

Re: [PATCH 2/5] kbuild: fix warning when domainname is not available

[Sam Ravnborg]

On Mon, Sep 14, 2009 at 01:04:11AM +0300, Felipe Contreras wrote:
> On Mon, Sep 14, 2009 at 12:58 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> > .
> > On Sun, Sep 13, 2009 at 17:42, Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> >> On Mon, Sep 14, 2009 at 12:12 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> >>> On Sun, Sep 13, 2009 at 15:38, Felipe Contreras wrote:
> >>>> +  if $domain; then
> >>>
> >>> is this really correct ?  i think you meant to use:
> >>> [ -n "$domain" ]
> >>
> >> What is the difference?
> >>
> >> $domain unset
> >> test -n "" -> false
> >> test -> false
> >>
> >> $domain is a valid string
> >> test -n "string" -> true
> >> test "string" -> true
> >
> > except that you didnt invoke `test` anywhere.  you're executing the
> > contents of $domain.
> 
> Ahh, I'll update it to:
> [ "$domain" ]

Please use [ -n "$domain" ].
Be explicit about what you do.

[Likewise in c we never omit "int" just because we can].

	Sam

Re: [PATCH 2/5] kbuild: fix warning when domainname is not available

[Felipe Contreras]

On Mon, Sep 14, 2009 at 7:46 AM, Sam Ravnborg <sam@ravnborg.org> wrote:
> On Mon, Sep 14, 2009 at 01:04:11AM +0300, Felipe Contreras wrote:
>> On Mon, Sep 14, 2009 at 12:58 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
>> > .
>> > On Sun, Sep 13, 2009 at 17:42, Felipe Contreras
>> > <felipe.contreras@gmail.com> wrote:
>> >> On Mon, Sep 14, 2009 at 12:12 AM, Mike Frysinger <vapier.adi@gmail.com> wrote:
>> >>> On Sun, Sep 13, 2009 at 15:38, Felipe Contreras wrote:
>> >>>> +  if $domain; then
>> >>>
>> >>> is this really correct ?  i think you meant to use:
>> >>> [ -n "$domain" ]
>> >>
>> >> What is the difference?
>> >>
>> >> $domain unset
>> >> test -n "" -> false
>> >> test -> false
>> >>
>> >> $domain is a valid string
>> >> test -n "string" -> true
>> >> test "string" -> true
>> >
>> > except that you didnt invoke `test` anywhere.  you're executing the
>> > contents of $domain.
>>
>> Ahh, I'll update it to:
>> [ "$domain" ]
>
> Please use [ -n "$domain" ].
> Be explicit about what you do.
>
> [Likewise in c we never omit "int" just because we can].

In fact 'int' is implicit of 'signed int', and 'long' is a shorthand
of 'signed long int' and so on. Also, AFAIK 'if (foo)' is preferred
over 'if (foo == true)' or 'if (foo != NULL)' and sometimes even 'if
(foo >= 0)'.

What's the point of going for the explicit form? Make the code less readable?

-- 
Felipe Contreras

Re: [PATCH 2/5] kbuild: fix warning when domainname is not available

[Mike Frysinger]

On Mon, Sep 14, 2009 at 04:25, Felipe Contreras wrote:
> On Mon, Sep 14, 2009 at 7:46 AM, Sam Ravnborg wrote:
>> On Mon, Sep 14, 2009 at 01:04:11AM +0300, Felipe Contreras wrote:
>>> On Mon, Sep 14, 2009 at 12:58 AM, Mike Frysinger wrote:
>>> > On Sun, Sep 13, 2009 at 17:42, Felipe Contreras wrote:
>>> >> On Mon, Sep 14, 2009 at 12:12 AM, Mike Frysinger wrote:
>>> >>> On Sun, Sep 13, 2009 at 15:38, Felipe Contreras wrote:
>>> >>>> +  if $domain; then
>>> >>>
>>> >>> is this really correct ?  i think you meant to use:
>>> >>> [ -n "$domain" ]
>>> >>
>>> >> What is the difference?
>>> >>
>>> >> $domain unset
>>> >> test -n "" -> false
>>> >> test -> false
>>> >>
>>> >> $domain is a valid string
>>> >> test -n "string" -> true
>>> >> test "string" -> true
>>> >
>>> > except that you didnt invoke `test` anywhere.  you're executing the
>>> > contents of $domain.
>>>
>>> Ahh, I'll update it to:
>>> [ "$domain" ]
>>
>> Please use [ -n "$domain" ].
>> Be explicit about what you do.
>>
>> [Likewise in c we never omit "int" just because we can].
>
> In fact 'int' is implicit of 'signed int', and 'long' is a shorthand
> of 'signed long int' and so on. Also, AFAIK 'if (foo)' is preferred
> over 'if (foo == true)' or 'if (foo != NULL)' and sometimes even 'if
> (foo >= 0)'.
>
> What's the point of going for the explicit form? Make the code less readable?

your argument here is the opposite of reality.  while some of us are
aware of implicit `test`behavior, not everyone is a shell scripting
master.  they look at [ "$foo" ] and dont immediately get the
intention.  or perhaps someone typoed and didnt actually want -n
semantics.  add the whole *3* characters and be done with it.
-mike

Re: [PATCH 4/5] acpi: fix trivial warning

[Daniel Walker]

On Sun, 2009-09-13 at 22:38 +0300, Felipe Contreras wrote:
> drivers/acpi/acpica/tbfadt.c: In function ‘acpi_tb_create_local_fadt’:
> arch/x86/include/asm/string_32.h:74: warning: array subscript is above array bounds
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Could you run this through checkpatch also, it looks like you have a few
style issues..

Daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] acpi: fix trivial warning

[Daniel Walker]

On Sun, 2009-09-13 at 22:38 +0300, Felipe Contreras wrote:
> drivers/acpi/acpica/tbfadt.c: In function ‘acpi_tb_create_local_fadt’:
> arch/x86/include/asm/string_32.h:74: warning: array subscript is above array bounds
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Could you run this through checkpatch also, it looks like you have a few
style issues..

Daniel

Re: [PATCH 4/5] acpi: fix trivial warning

[Felipe Contreras]

On Mon, Sep 14, 2009 at 4:55 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> On Sun, 2009-09-13 at 22:38 +0300, Felipe Contreras wrote:
>> drivers/acpi/acpica/tbfadt.c: In function ‘acpi_tb_create_local_fadt’:
>> arch/x86/include/asm/string_32.h:74: warning: array subscript is above array bounds
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> Could you run this through checkpatch also, it looks like you have a few
> style issues..

I did... the style issues are already there, my patch is not
introducing them. Do you want me to send a separate patch to fix the
existing style issues?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] acpi: fix trivial warning

[Felipe Contreras]

On Mon, Sep 14, 2009 at 4:55 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> On Sun, 2009-09-13 at 22:38 +0300, Felipe Contreras wrote:
>> drivers/acpi/acpica/tbfadt.c: In function ‘acpi_tb_create_local_fadt’:
>> arch/x86/include/asm/string_32.h:74: warning: array subscript is above array bounds
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> Could you run this through checkpatch also, it looks like you have a few
> style issues..

I did... the style issues are already there, my patch is not
introducing them. Do you want me to send a separate patch to fix the
existing style issues?

-- 
Felipe Contreras

Re: [PATCH 4/5] acpi: fix trivial warning

[Daniel Walker]

On Mon, 2009-09-14 at 17:34 +0300, Felipe Contreras wrote:
> On Mon, Sep 14, 2009 at 4:55 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> > On Sun, 2009-09-13 at 22:38 +0300, Felipe Contreras wrote:
> >> drivers/acpi/acpica/tbfadt.c: In function ‘acpi_tb_create_local_fadt’:
> >> arch/x86/include/asm/string_32.h:74: warning: array subscript is above array bounds
> >>
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >
> > Could you run this through checkpatch also, it looks like you have a few
> > style issues..
> 
> I did... the style issues are already there, my patch is not
> introducing them. Do you want me to send a separate patch to fix the
> existing style issues?

Yes, If your inclined to clean up the whole file, or just the
surrounding code that would be helpful ..

Daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] acpi: fix trivial warning

[Daniel Walker]

On Mon, 2009-09-14 at 17:34 +0300, Felipe Contreras wrote:
> On Mon, Sep 14, 2009 at 4:55 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> > On Sun, 2009-09-13 at 22:38 +0300, Felipe Contreras wrote:
> >> drivers/acpi/acpica/tbfadt.c: In function ‘acpi_tb_create_local_fadt’:
> >> arch/x86/include/asm/string_32.h:74: warning: array subscript is above array bounds
> >>
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >
> > Could you run this through checkpatch also, it looks like you have a few
> > style issues..
> 
> I did... the style issues are already there, my patch is not
> introducing them. Do you want me to send a separate patch to fix the
> existing style issues?

Yes, If your inclined to clean up the whole file, or just the
surrounding code that would be helpful ..

Daniel

Re: [PATCH 2/5] kbuild: fix warning when domainname is not available

[Felipe Contreras]

On Mon, Sep 14, 2009 at 4:43 PM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> On Mon, Sep 14, 2009 at 04:25, Felipe Contreras wrote:
>> On Mon, Sep 14, 2009 at 7:46 AM, Sam Ravnborg wrote:
>>> On Mon, Sep 14, 2009 at 01:04:11AM +0300, Felipe Contreras wrote:
>>>> On Mon, Sep 14, 2009 at 12:58 AM, Mike Frysinger wrote:
>>>> > On Sun, Sep 13, 2009 at 17:42, Felipe Contreras wrote:
>>>> >> On Mon, Sep 14, 2009 at 12:12 AM, Mike Frysinger wrote:
>>>> >>> On Sun, Sep 13, 2009 at 15:38, Felipe Contreras wrote:
>>>> >>>> +  if $domain; then
>>>> >>>
>>>> >>> is this really correct ?  i think you meant to use:
>>>> >>> [ -n "$domain" ]
>>>> >>
>>>> >> What is the difference?
>>>> >>
>>>> >> $domain unset
>>>> >> test -n "" -> false
>>>> >> test -> false
>>>> >>
>>>> >> $domain is a valid string
>>>> >> test -n "string" -> true
>>>> >> test "string" -> true
>>>> >
>>>> > except that you didnt invoke `test` anywhere.  you're executing the
>>>> > contents of $domain.
>>>>
>>>> Ahh, I'll update it to:
>>>> [ "$domain" ]
>>>
>>> Please use [ -n "$domain" ].
>>> Be explicit about what you do.
>>>
>>> [Likewise in c we never omit "int" just because we can].
>>
>> In fact 'int' is implicit of 'signed int', and 'long' is a shorthand
>> of 'signed long int' and so on. Also, AFAIK 'if (foo)' is preferred
>> over 'if (foo == true)' or 'if (foo != NULL)' and sometimes even 'if
>> (foo >= 0)'.
>>
>> What's the point of going for the explicit form? Make the code less readable?
>
> your argument here is the opposite of reality.  while some of us are
> aware of implicit `test`behavior, not everyone is a shell scripting
> master.  they look at [ "$foo" ] and dont immediately get the
> intention.  or perhaps someone typoed and didnt actually want -n
> semantics.  add the whole *3* characters and be done with it.

I consider myself an expert in bash (or at least was some time ago)
and I still need to run 'man bash' in order to see what the hell -n
means. On the other hand, what can [ "$foo" ] be confused with? To me
that can be assumed as: $foo is valid.

In any case, I don't see that idiom being used in the source tree
(which I think is bad), and I see -n being used in some places, so
I'll send a new patch using -n for consistency.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 4/5] acpi: fix trivial warning

[Felipe Contreras]

On Mon, Sep 14, 2009 at 5:02 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> On Mon, 2009-09-14 at 17:34 +0300, Felipe Contreras wrote:
>> I did... the style issues are already there, my patch is not
>> introducing them. Do you want me to send a separate patch to fix the
>> existing style issues?
>
> Yes, If your inclined to clean up the whole file, or just the
> surrounding code that would be helpful ..

Nobody seems to like the patch you suggested me to do... did I wasted
my time doing it?
http://article.gmane.org/gmane.linux.acpi.devel/42383

-- 
Felipe Contreras

Re: [PATCH 4/5] acpi: fix trivial warning

[Daniel Walker]

On Thu, 2009-11-12 at 02:23 +0200, Felipe Contreras wrote:
> On Mon, Sep 14, 2009 at 5:02 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> > On Mon, 2009-09-14 at 17:34 +0300, Felipe Contreras wrote:
> >> I did... the style issues are already there, my patch is not
> >> introducing them. Do you want me to send a separate patch to fix the
> >> existing style issues?
> >
> > Yes, If your inclined to clean up the whole file, or just the
> > surrounding code that would be helpful ..
> 
> Nobody seems to like the patch you suggested me to do... did I wasted
> my time doing it?
> http://article.gmane.org/gmane.linux.acpi.devel/42383

Did you get some negative feedback ? I don't see any..

Daniel

Re: [PATCH 4/5] acpi: fix trivial warning

[Felipe Contreras]

On Fri, Nov 13, 2009 at 7:43 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> On Thu, 2009-11-12 at 02:23 +0200, Felipe Contreras wrote:
>> On Mon, Sep 14, 2009 at 5:02 PM, Daniel Walker <dwalker@fifo99.com> wrote:
>> > On Mon, 2009-09-14 at 17:34 +0300, Felipe Contreras wrote:
>> >> I did... the style issues are already there, my patch is not
>> >> introducing them. Do you want me to send a separate patch to fix the
>> >> existing style issues?
>> >
>> > Yes, If your inclined to clean up the whole file, or just the
>> > surrounding code that would be helpful ..
>>
>> Nobody seems to like the patch you suggested me to do... did I wasted
>> my time doing it?
>> http://article.gmane.org/gmane.linux.acpi.devel/42383
>
> Did you get some negative feedback ? I don't see any..

Well:
---
And after today's discussion on kernel summit on this topic, I wouldn't
expect any maintainer to merge it, sorry :)
---

That doesn't seem too positive. Or at least there's no indication that
somebody will pick it up.

-- 
Felipe Contreras

Re: [PATCH 4/5] acpi: fix trivial warning

[Thiago Farina]

On Fri, Nov 13, 2009 at 6:28 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Fri, Nov 13, 2009 at 7:43 PM, Daniel Walker <dwalker@fifo99.com> wrote:
>> On Thu, 2009-11-12 at 02:23 +0200, Felipe Contreras wrote:
>>> On Mon, Sep 14, 2009 at 5:02 PM, Daniel Walker <dwalker@fifo99.com> wrote:
>>> > On Mon, 2009-09-14 at 17:34 +0300, Felipe Contreras wrote:
>>> >> I did... the style issues are already there, my patch is not
>>> >> introducing them. Do you want me to send a separate patch to fix the
>>> >> existing style issues?
>>> >
>>> > Yes, If your inclined to clean up the whole file, or just the
>>> > surrounding code that would be helpful ..
>>>
>>> Nobody seems to like the patch you suggested me to do... did I wasted
>>> my time doing it?
>>> http://article.gmane.org/gmane.linux.acpi.devel/42383
>>
>> Did you get some negative feedback ? I don't see any..
>
> Well:
> ---
> And after today's discussion on kernel summit on this topic, I wouldn't
> expect any maintainer to merge it, sorry :)
> ---
>
> That doesn't seem too positive. Or at least there's no indication that
> somebody will pick it up.

I think it is because it doesn't "aggregate" real value to the kernel
(like fixing a crash, that adds value), and the other guys has more
things to do, and don't have time to waste. But that can not be the
true, this is just my point of view.

Re: [PATCH 4/5] acpi: fix trivial warning

[Daniel Walker]

On Fri, 2009-11-13 at 22:28 +0200, Felipe Contreras wrote:
> On Fri, Nov 13, 2009 at 7:43 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> > On Thu, 2009-11-12 at 02:23 +0200, Felipe Contreras wrote:
> >> On Mon, Sep 14, 2009 at 5:02 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> >> > On Mon, 2009-09-14 at 17:34 +0300, Felipe Contreras wrote:
> >> >> I did... the style issues are already there, my patch is not
> >> >> introducing them. Do you want me to send a separate patch to fix the
> >> >> existing style issues?
> >> >
> >> > Yes, If your inclined to clean up the whole file, or just the
> >> > surrounding code that would be helpful ..
> >>
> >> Nobody seems to like the patch you suggested me to do... did I wasted
> >> my time doing it?
> >> http://article.gmane.org/gmane.linux.acpi.devel/42383
> >
> > Did you get some negative feedback ? I don't see any..
> 
> Well:
> ---
> And after today's discussion on kernel summit on this topic, I wouldn't
> expect any maintainer to merge it, sorry :)
> ---
> 
> That doesn't seem too positive. Or at least there's no indication that
> somebody will pick it up.


I'd re-submit with a better description of the patch. You should always
try to describe what your doing as accurately as possible so the
maintainer doesn't have to work very hard to know what your doing.. That
particular patch just has a one liner description that wasn't very
informative .. Either that or re-submit your series without that patch
if you don't have confidence in it.

Daniel

Re: [PATCH 4/5] acpi: fix trivial warning

[Felipe Contreras]

On Sun, Nov 15, 2009 at 7:15 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> On Fri, 2009-11-13 at 22:28 +0200, Felipe Contreras wrote:
>> That doesn't seem too positive. Or at least there's no indication that
>> somebody will pick it up.
>
> I'd re-submit with a better description of the patch. You should always
> try to describe what your doing as accurately as possible so the
> maintainer doesn't have to work very hard to know what your doing.

There's nothing to add. It's a patch to cleanup the coding style, that's it.

> That
> particular patch just has a one liner description that wasn't very
> informative .. Either that or re-submit your series without that patch
> if you don't have confidence in it.

All of my patches have been picked up, except the ones for ACPI. I
haven't received a single comment from them, which would explain the
current state of the code.

If somebody raised the hand and said; I'll merge this, please resend,
I'd do that, otherwise I think it's a waste of time.

-- 
Felipe Contreras

Re: [PATCH 4/5] acpi: fix trivial warning

[Daniel Walker]

On Sun, 2009-11-15 at 19:27 +0200, Felipe Contreras wrote:
> On Sun, Nov 15, 2009 at 7:15 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> > On Fri, 2009-11-13 at 22:28 +0200, Felipe Contreras wrote:
> >> That doesn't seem too positive. Or at least there's no indication that
> >> somebody will pick it up.
> >
> > I'd re-submit with a better description of the patch. You should always
> > try to describe what your doing as accurately as possible so the
> > maintainer doesn't have to work very hard to know what your doing.
> 
> There's nothing to add. It's a patch to cleanup the coding style, that's it.

How do you know the coding style isn't correct? Did you just visually
inspect it?

> > That
> > particular patch just has a one liner description that wasn't very
> > informative .. Either that or re-submit your series without that patch
> > if you don't have confidence in it.
> 
> All of my patches have been picked up, except the ones for ACPI. I
> haven't received a single comment from them, which would explain the
> current state of the code.
> 
> If somebody raised the hand and said; I'll merge this, please resend,
> I'd do that, otherwise I think it's a waste of time.

Patches often times aren't accepted in the first submission, could be
for lots of reasons. It's your series tho ..

Daniel

Re: [PATCH 4/5] acpi: fix trivial warning

[Alexey Starikovskiy]

You've made changes to automatically generated files, so all the style 
changes
would be overwritten when next time these files are generated -- all the 
files go through
Lindent, and almost all your changes were introduced by it. If you 
really want to make
style changes to ACPICA files, try to play with Lindent, so it produce 
what you want.
If you have anything beside style patches, please submit them separately

Regards,
Alex.

Daniel Walker wrote:
> On Sun, 2009-11-15 at 19:27 +0200, Felipe Contreras wrote:
>   
>> On Sun, Nov 15, 2009 at 7:15 PM, Daniel Walker <dwalker@fifo99.com> wrote:
>>     
>>> On Fri, 2009-11-13 at 22:28 +0200, Felipe Contreras wrote:
>>>       
>>>> That doesn't seem too positive. Or at least there's no indication that
>>>> somebody will pick it up.
>>>>         
>>> I'd re-submit with a better description of the patch. You should always
>>> try to describe what your doing as accurately as possible so the
>>> maintainer doesn't have to work very hard to know what your doing.
>>>       
>> There's nothing to add. It's a patch to cleanup the coding style, that's it.
>>     
>
> How do you know the coding style isn't correct? Did you just visually
> inspect it?
>
>   
>>> That
>>> particular patch just has a one liner description that wasn't very
>>> informative .. Either that or re-submit your series without that patch
>>> if you don't have confidence in it.
>>>       
>> All of my patches have been picked up, except the ones for ACPI. I
>> haven't received a single comment from them, which would explain the
>> current state of the code.
>>
>> If somebody raised the hand and said; I'll merge this, please resend,
>> I'd do that, otherwise I think it's a waste of time.
>>     
>
> Patches often times aren't accepted in the first submission, could be
> for lots of reasons. It's your series tho ..
>
> Daniel
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [PATCH 4/5] acpi: fix trivial warning

[Daniel Walker]

On Sun, 2009-11-15 at 20:54 +0300, Alexey Starikovskiy wrote:
> You've made changes to automatically generated files, so all the style 
> changes
> would be overwritten when next time these files are generated -- all the 
> files go through
> Lindent, and almost all your changes were introduced by it. If you 
> really want to make
> style changes to ACPICA files, try to play with Lindent, so it produce 
> what you want.
> If you have anything beside style patches, please submit them separately
> 

AFAIK, Lindent can't produce checkpatch clean output. It has to do with
various short comings in indent. You really need a human to check up on
it.

Daniel

Re: [PATCH 4/5] acpi: fix trivial warning

[Daniel Walker]

On Sun, 2009-11-15 at 21:10 +0300, Alexey Starikovskiy wrote:

> >   
> It is not really possible. Lindent is not the last step in the process.

What are the steps on the process?

Daniel

Re: [PATCH 4/5] acpi: fix trivial warning

[Alexey Starikovskiy]

Daniel Walker wrote:
> On Sun, 2009-11-15 at 20:54 +0300, Alexey Starikovskiy wrote:
>   
>> You've made changes to automatically generated files, so all the style 
>> changes
>> would be overwritten when next time these files are generated -- all the 
>> files go through
>> Lindent, and almost all your changes were introduced by it. If you 
>> really want to make
>> style changes to ACPICA files, try to play with Lindent, so it produce 
>> what you want.
>> If you have anything beside style patches, please submit them separately
>>
>>     
>
> AFAIK, Lindent can't produce checkpatch clean output. It has to do with
> various short comings in indent. You really need a human to check up on
> it.
>
> Daniel
>
>   
It is not really possible. Lindent is not the last step in the process.

Re: [PATCH 4/5] acpi: fix trivial warning

[Lin Ming]

On Mon, 2009-11-16 at 01:27 +0800, Felipe Contreras wrote:
> On Sun, Nov 15, 2009 at 7:15 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> > On Fri, 2009-11-13 at 22:28 +0200, Felipe Contreras wrote:
> >> That doesn't seem too positive. Or at least there's no indication that
> >> somebody will pick it up.
> >
> > I'd re-submit with a better description of the patch. You should always
> > try to describe what your doing as accurately as possible so the
> > maintainer doesn't have to work very hard to know what your doing.
> 
> There's nothing to add. It's a patch to cleanup the coding style, that's it.
> 
> > That
> > particular patch just has a one liner description that wasn't very
> > informative .. Either that or re-submit your series without that patch
> > if you don't have confidence in it.
> 
> All of my patches have been picked up, except the ones for ACPI. I
> haven't received a single comment from them, which would explain the
> current state of the code.

Hi, Felipe

All files under linux-2.6/drivers/acpi/acpica and most files under
linux-2.6/include/acpi are ACPICA code, see http://www.acpica.org for
detail.

The ACPICA's coding style is totally different than linux kernel.
For example, a function named AcpiEvaluateObject in ACPICA, but in linux
kernel it is called acpi_evaluate_object.

We have a program "acpisrc" to convert the ACPICA style code to linux
style and then run lindent on the converted code.

You can download the official ACPICA release from
http://www.acpica.org/downloads/ , "acpisrc" is included in it.

It's great that you are contributing to ACPICA code.
All these linuxized ACPICA codes are generated by acpisrc and lindent,
so we'd better not touch those code directly.

Instead, our process is:
1. Write patches for ACPICA code.
2. Send patches to devel.acpica.org, robert.moore@intel.com, ming.m.lin@intel.com
3. I will generate and send the linuxized ACPICA code to Len Brown <lenb@kernel.org>
4. Len asks Linus to merge the linuxized ACPICA code.

Thanks,
Lin Ming

> 
> If somebody raised the hand and said; I'll merge this, please resend,
> I'd do that, otherwise I think it's a waste of time.
>