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.
