[PATCH v2]ata:ahci.c Fix warning: comparison between 'enum <anonymous>' and 'enum <anonymous>'

The below patch fixes a warning message during compiling of the kernel
  CC      drivers/ata/ahci.o
drivers/ata/ahci.c: In function 'ahci_init_one':
drivers/ata/ahci.c:1045:2: warning: comparison between 'enum <anonymous>' and 'enum <anonymous>'

 Signed-off-by: Justin P. Mattock [off-list ref]

---
 drivers/ata/ahci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 8ca16f5..68c9add 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c

@@ -1038,11 +1038,11 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	struct device *dev = &pdev->dev;
 	struct ahci_host_priv *hpriv;
 	struct ata_host *host;
-	int n_ports, i, rc;
+	int n_ports, i, rc, sum = AHCI_MAX_CMDS;
 
 	VPRINTK("ENTER\n");
 
-	WARN_ON(ATA_MAX_QUEUE > AHCI_MAX_CMDS);
+	WARN_ON(ATA_MAX_QUEUE > sum);
 
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");

-- 
1.7.1.rc1.21.gf3bd6

Hello,

On 06/30/2010 08:37 AM, Justin P. Mattock wrote:

The below patch fixes a warning message during compiling of the kernel
  CC      drivers/ata/ahci.o
drivers/ata/ahci.c: In function 'ahci_init_one':
drivers/ata/ahci.c:1045:2: warning: comparison between 'enum <anonymous>' and 'enum <anonymous>'

 Signed-off-by: Justin P. Mattock [off-list ref]

Hmm, is this something we wanna go around the kernel and updating
sources?  Or should we just flick a gcc option?

And, just do WARN_ON((int)ATA_MAX_QUEUE > (int)AHCI_MAX_CMDS)

Thanks.

-- 
tejun

On 06/29/2010 11:49 PM, Tejun Heo wrote:

Hello,

On 06/30/2010 08:37 AM, Justin P. Mattock wrote:

quoted

The below patch fixes a warning message during compiling of the kernel
   CC      drivers/ata/ahci.o
drivers/ata/ahci.c: In function 'ahci_init_one':
drivers/ata/ahci.c:1045:2: warning: comparison between 'enum<anonymous>' and 'enum<anonymous>'

  Signed-off-by: Justin P. Mattock[off-list ref]

Hmm, is this something we wanna go around the kernel and updating
sources?  Or should we just flick a gcc option?

the flick a gcc option seems easier todo, but my guess its probably not the right way of dealing with the issue(even a warning).

quoted

@@ -1038,11 +1038,11 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
  	struct device *dev =&pdev->dev;
  	struct ahci_host_priv *hpriv;
  	struct ata_host *host;
-	int n_ports, i, rc;
+	int n_ports, i, rc, sum = AHCI_MAX_CMDS;

  	VPRINTK("ENTER\n");

-	WARN_ON(ATA_MAX_QUEUE>  AHCI_MAX_CMDS);
+	WARN_ON(ATA_MAX_QUEUE>  sum);

And, just do WARN_ON((int)ATA_MAX_QUEUE>  (int)AHCI_MAX_CMDS)

Thanks.

that builds clean. keep in mind I just compile tested, no rebooting or anything. Should I just resend with what you posted, and call it that, or is this something that needs more?

Justin P. Mattock

On 06/30/2010 09:06 AM, Justin P. Mattock wrote:

quoted

Hmm, is this something we wanna go around the kernel and updating
sources?  Or should we just flick a gcc option?

the flick a gcc option seems easier todo, but my guess its probably not
the right way of dealing with the issue(even a warning).

What does the extra warning buy us?  There are several places which
use anonymous enums for constants and I can't see what the benefit of
this warning would be.

quoted

And, just do WARN_ON((int)ATA_MAX_QUEUE > (int)AHCI_MAX_CMDS)

that builds clean. keep in mind I just compile tested, no rebooting or
anything. Should I just resend with what you posted, and call it that,
or is this something that needs more?

I think it would be better to first decide what to do about the
new warnings.

Thanks.

-- 
tejun

On 06/30/2010 12:11 AM, Tejun Heo wrote:

On 06/30/2010 09:06 AM, Justin P. Mattock wrote:

quoted

quoted

Hmm, is this something we wanna go around the kernel and updating
sources?  Or should we just flick a gcc option?

the flick a gcc option seems easier todo, but my guess its probably not
the right way of dealing with the issue(even a warning).

What does the extra warning buy us?  There are several places which
use anonymous enums for constants and I can't see what the benefit of
this warning would be.

I don't think it buys us anything..think it's just saying "hey you have two #defines with the same value" or something in that area(if Im reading the warning correctly)  funny thing is, is gcc should of done the same with the original patch that I sent, as well as the second..
(but could be wrong).

quoted

quoted

And, just do WARN_ON((int)ATA_MAX_QUEUE>  (int)AHCI_MAX_CMDS)

that builds clean. keep in mind I just compile tested, no rebooting or
anything. Should I just resend with what you posted, and call it that,
or is this something that needs more?

I think it would be better to first decide what to do about the
new warnings.

Thanks.

sure.. no problem.

cheers,

Justin P. Mattock

Hello,

On 06/30/2010 09:21 AM, Justin P. Mattock wrote:

quoted

What does the extra warning buy us?  There are several places which
use anonymous enums for constants and I can't see what the benefit of
this warning would be.

I don't think it buys us anything..think it's just saying "hey
you have two #defines with the same value" or something in that
area(if Im reading the warning correctly) funny thing is, is
gcc should of done the same with the original patch that I
sent, as well as the second..  (but could be wrong).

Okay, just tested and it's warning that the code is comparing two
enums which are from two different enum blocks.  I'm kind of
doubtful about its usefulness but then again I don't think there
will be many occassions of this to be problematic either.  For
now, can you please submit a patch to cast just ATA_MAX_QUEUE
to (int) before comparing and short comment explanining why it's
necessary?

Thanks.

-- 
tejun

On 07/03/2010 02:57 AM, Tejun Heo wrote:

Hello,

On 06/30/2010 09:21 AM, Justin P. Mattock wrote:

quoted

quoted

What does the extra warning buy us?  There are several places which
use anonymous enums for constants and I can't see what the benefit of
this warning would be.

I don't think it buys us anything..think it's just saying "hey
you have two #defines with the same value" or something in that
area(if Im reading the warning correctly) funny thing is, is
gcc should of done the same with the original patch that I
sent, as well as the second..  (but could be wrong).

Okay, just tested and it's warning that the code is comparing two
enums which are from two different enum blocks.  I'm kind of
doubtful about its usefulness but then again I don't think there
will be many occassions of this to be problematic either.  For
now, can you please submit a patch to cast just ATA_MAX_QUEUE
to (int) before comparing and short comment explanining why it's
necessary?

Thanks.

o.k., just sent a patch subject is:
Fix warning: comparison between 'enum <anonymous>' and 'enum <anonymous>'
hopefully it's doing what you had asked it to do i.g. send
ATA_MAX_QUEUE to int.

let me know if it needs to be resent or anything.

Justin P. Mattock

On 07/03/2010 02:57 AM, Tejun Heo wrote:

Hello,

On 06/30/2010 09:21 AM, Justin P. Mattock wrote:

quoted

quoted

What does the extra warning buy us?  There are several places which
use anonymous enums for constants and I can't see what the benefit of
this warning would be.

I don't think it buys us anything..think it's just saying "hey
you have two #defines with the same value" or something in that
area(if Im reading the warning correctly) funny thing is, is
gcc should of done the same with the original patch that I
sent, as well as the second..  (but could be wrong).

Okay, just tested and it's warning that the code is comparing two
enums which are from two different enum blocks.  I'm kind of
doubtful about its usefulness but then again I don't think there
will be many occassions of this to be problematic either.  For
now, can you please submit a patch to cast just ATA_MAX_QUEUE
to (int) before comparing and short comment explanining why it's
necessary?

Thanks.

o.k. so it's what you where asking..

cheers,

Justin P. Mattock