Re: [kvm-unit-tests PATCH v2 5/5] s390x: SCLP unit test

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

 



On Fri, 8 Nov 2019 10:35:32 +0100
Thomas Huth <thuth@xxxxxxxxxx> wrote:

> On 04/11/2019 12.19, Claudio Imbrenda wrote:
> > On Mon, 4 Nov 2019 10:45:07 +0100
> > Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:  
> [...]
> >>> +static void test_toolong(void)
> >>> +{
> >>> +	uint32_t cmd = SCLP_CMD_WRITE_EVENT_DATA;
> >>> +	uint16_t res = SCLP_RC_SCCB_BOUNDARY_VIOLATION;  
> >>
> >> Why use variables for constants that are never touched?  
> > 
> > readability mostly. the names of the constants are rather long.
> > the compiler will notice it and do the Right Thing™  
> 
> I'd like to suggest to add the "const" keyword to both variables in
> that case, then it's clear that they are not used to be modified.

good point

> >>> +		h->length = 4096;
> >>> +
> >>> +		valid_code = commands[i];
> >>> +		cc = sclp_service_call(commands[i], h);
> >>> +		if (cc)
> >>> +			break;
> >>> +		if (h->response_code ==
> >>> SCLP_RC_NORMAL_READ_COMPLETION)
> >>> +			return;
> >>> +		if (h->response_code !=
> >>> SCLP_RC_INVALID_SCLP_COMMAND)
> >>> +			break;  
> >>
> >> Depending on line length you could add that to the cc check.
> >> Maybe you could also group the error conditions before the success
> >> conditions or the other way around.  
> > 
> > yeah it woud fit, but I'm not sure it would be more readable:
> > 
> > if (cc || (h->response_code != SCLP_RC_INVALID_SCLP_COMMAND))
> >                          break;  
> 
> In case you go with that solution, please drop the innermost
> parentheses.

why so much hatred for parentheses? :D

but no, I'm not going to do it, it's not just less readable, it's
actually wrong!

SCLP_RC_NORMAL_READ_COMPLETION != SCLP_RC_INVALID_SCLP_COMMAND

the correct version would be:

if (cc ||
	h->response_code != SCLP_RC_INVALID_SCLP_COMMAND &&
	h->response_code != SCLP_RC_NORMAL_READ_COMPLETION)

which is more lines, and significantly less readable.




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux