Re: [PATCH] rdma\hfi1 - coding style issues cleanup

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 12, 2016 at 04:25:38PM -0400, Doug Ledford wrote:
On 04/28/2016 08:41 AM, Dennis Dalessandro wrote:
On Thu, Apr 28, 2016 at 10:58:20AM +0300, omer.dagan@xxxxxxxxxxx wrote:
From: Omer Dagan <omer.dagan@xxxxxxxxxxx>

diff --git a/drivers/staging/rdma/hfi1/aspm.h
b/drivers/staging/rdma/hfi1/aspm.h
index 0d58fe3..384df0a 100644
--- a/drivers/staging/rdma/hfi1/aspm.h
+++ b/drivers/staging/rdma/hfi1/aspm.h
@@ -234,7 +234,7 @@ static inline void aspm_disable_all(struct
hfi1_devdata *dd)
{
    struct hfi1_ctxtdata *rcd;
    unsigned long flags;
-    unsigned i;
+    unsigned int i;

Can you please explain the value in doing "unsigned int" vs "unsigned"?
None of the static checkers like checkpatch, sparse, or coccinelle seem
to mind.

I'm not a big fan of all this churn for something that means next to
nothing.  The C language specifies that unsigned means unsigned int,
adding int is actually redundant and makes code lines longer to read.

I completely agree with Doug. This is a lot of churn for something that doesn't really add much value.

To be fair though, I do want to correct my statement above, apparently checkpatch *does* indeed throw a warning for this, was just checked in a few weeks back:

a1ce18e4f941("checkpatch: warn on bare unsigned or signed declarations without int")

Since the hfi1 driver is in staging, it's probably a big target for stuff like this. The git log for the above checkpatch commit just says kernel style prefers it this way. I don't see any justification for why and I fail to see the benefit so I would prefer we drop this part of the patch.

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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux