RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of Richard Cochran
Sent: Thursday, August 23, 2012 10:30 AM
To: Vick, Matthew
Cc: Kirsher, Jeffrey T; davem@davemloft.net; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

On Thu, Aug 23, 2012 at 04:09:30PM +0000, Vick, Matthew wrote:

quoted

quoted

-----Original Message-----
From: Richard Cochran [mailto:richardcochran@gmail.com]
Sent: Thursday, August 23, 2012 4:04 AM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Vick, Matthew; netdev@vger.kernel.org;
gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for

CONFIG_IGB_PTP.

quoted

quoted

On Thu, Aug 23, 2012 at 02:56:50AM -0700, Jeff Kirsher wrote:

quoted

From: Matthew Vick <redacted>

For users without CONFIG_IGB_PTP=y, we should not be compiling any

PTP

quoted

code into the driver. Tidy up the wrapping in igb to support this.

Actually, you are doing more than that. You are adding a bunch of
comments onto the already existing #endifs.

Fair enough. Would you like me to update the patch description?

Better would be to drop of the pendantic #endif /*CONFIG_FOO*/ stuff.
It is just churn.

Personally disagree here. I do agree that the churn is annoying with how it breaks git blame, however, in general I prefer tags at the end of #ifdefs even for short ones because it increases my ability to quickly spot matches. The end comment aligns with the starting comment, and even for small blocks makes it easier to process. It is less necessary the smaller the block, but I always prefer to have it than not.

That said, it is nice when git blame points to the right place, and the comments aren't super necessary for such short blocks.

- Jake

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