Re: [PATCH] Staging: bcm: CmHost: unline split quoted strings.

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

 





On Mon, 17 Mar 2014, Mark Einon wrote:

On Sun, Mar 16, 2014 at 10:16:09PM -0400, Gary Rookard wrote:


On Sun, 16 Mar 2014, Gary Rookard wrote:



On Sun, 16 Mar 2014, Mark Einon wrote:

On Sun, Mar 16, 2014 at 02:19:51PM -0400, Gary Rookard wrote:
I unline split some quoted strings.

Hi,

Why? What issue does it fix?

--
Hi,

It fixes a checkpatch error/warning "quoted string split across
lines" or something phrased very simular. Is there
a problem with.

--
Hi again,

Well firstly I do know that there is a trade off going on with the patch
that way, but for me, the general rule of thumb is that not sometimes
but most of the time you want your strings avalable for grep. And with
them split across lines it is hard -> impossible to accomplish. Thus
line length becomes meaningless or should I say not quit as bad as
it sounds. If you look they are sill shorter then alot of the code
in that
region of the file.
--

Hi Gary,

Ok. Fixing the split line issue is good - there's no problems with what you're
trying to do there.

However:

* The commit message should explain both what you're doing, and why you're
doing it - this makes reviewing the patch and referencing in future far easier.
For a checkpatch fix, you would normally expect checkpatch to be mentioned and
the issues that are being addressed - cut and pasting the checkpatch output
would work in this case, as it's not too verbose.
(Also applies to your other patch).

* You've fixed one checkpatch issue, but introduced another - meaning that
* another patch is probably needed. Why not make the code more readable while
* you're at it?

Cheers,

Mark

--
Hi,

Agreed, my commit messages are sorely lacking in
appropriate content. Will fix these up, and in the
future try to be more mindful about.

Thanks,
Gary










Signed-off-by: Gary Alan Rookard <garyrookard@xxxxxxxxx>

---
On branch staging-next
drivers/staging/bcm/CmHost.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/bcm/CmHost.c b/drivers/staging/bcm/CmHost.c
index 2d1f94d..4cb59d7 100644
--- a/drivers/staging/bcm/CmHost.c
+++ b/drivers/staging/bcm/CmHost.c
@@ -975,8 +975,7 @@ static VOID DumpCmControlPacket(PVOID pvBuffer)
				psfCSType->cCPacketClassificationRule.u8EthernetDestMacAddressLength);

		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL,
-				DBG_LVL_ALL, "u8EthernetSourceMACAddress[6]: "
-				"%pM", psfCSType->cCPacketClassificationRule.
+				DBG_LVL_ALL, "u8EthernetSourceMACAddress[6]: %pM",
psfCSType->cCPacketClassificationRule.
						u8EthernetSourceMACAddress);

You've also gone way over the 80 char limit for the line, for no
real reason.
if anything, I would consider making
psfCSType->cCPacketClassificationRule.u8EthernetSourceMACAddress
more readable.
Similarly for the changes below.

Cheers,

Mark


		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL,
DBG_LVL_ALL, "u8EthertypeLength: 0x%02X ",
@@ -1092,8 +1091,7 @@ static VOID DumpCmControlPacket(PVOID pvBuffer)
				psfCSType->cCPacketClassificationRule.u8ProtocolSourcePortRangeLength);

		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL,
-				DBG_LVL_ALL, "u8ProtocolSourcePortRange[4]: "
-				"0x%*ph ", 4, psfCSType->
+				DBG_LVL_ALL, "u8ProtocolSourcePortRange[4]: 0x%*ph ", 4,
psfCSType->
						cCPacketClassificationRule.
						u8ProtocolSourcePortRange);

@@ -1101,8 +1099,7 @@ static VOID DumpCmControlPacket(PVOID pvBuffer)
				psfCSType->cCPacketClassificationRule.u8ProtocolDestPortRangeLength);

		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL,
-				DBG_LVL_ALL, "u8ProtocolDestPortRange[4]: "
-				"0x%*ph ", 4, psfCSType->
+				DBG_LVL_ALL, "u8ProtocolDestPortRange[4]: 0x%*ph ", 4,
psfCSType->
						cCPacketClassificationRule.
						u8ProtocolDestPortRange);

@@ -1118,8 +1115,7 @@ static VOID DumpCmControlPacket(PVOID pvBuffer)
				psfCSType->cCPacketClassificationRule.u8EthernetDestMacAddressLength);

		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL,
-				DBG_LVL_ALL, "u8EthernetSourceMACAddress[6]: "
-				"%pM", psfCSType->cCPacketClassificationRule.
+				DBG_LVL_ALL, "u8EthernetSourceMACAddress[6]: %pM",
psfCSType->cCPacketClassificationRule.
						u8EthernetSourceMACAddress);

		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, DUMP_CONTROL,
DBG_LVL_ALL, "u8EthertypeLength: 0x%02X ",
psfCSType->cCPacketClassificationRule.u8EthertypeLength);
--
1.9.0

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux