Re: [hackers] [slstatus][PATCH keymap 1/2] Add keymap component

From: Aaron Marcher <me_AT_drkhsh.at>
Date: Wed, 23 May 2018 00:29:33 +0200

Michael,

Thank you very much for your great work. However I have a few things for
you to change.

> - components/backlight\

>-/* backlight */
>-const char *backlight_perc(const char *);
>-

Why are you removing backlight from Makefile and slstatus.h?

> + for (i = 0; i < sizeof(EXCLUDES)/sizeof(EXCLUDES[0]); ++i)
> + if (strstr(sym, EXCLUDES[i]))
> + return 0;

According to coding style: "Use block for single statement ifs".

> +/* Given a xkb_symbols string (syms) retrieve
> + * and populate the provided layout and variant
> + * strings
> + */

I think this specific comment is unnecessary as the function explains
itself.

> + char *token,
> + *copy,
> + *delims = "+:";

I don't think that the newlines are necessary here, it can be put in one
line. Additionally the indentation is wrong. And try to only declare
variables at the beginning of a block and initialize them later.

> + if (IsLayoutOrVariant(token)
> + && !(strlen(token) == 1 && isdigit(token[0]))) {

Indentation is wrong here, tabs to the last indent and spaces for
alignment afterwards.

> +const char *
> +keymap(void)
> +{
> + static char layout[LAYOUT_MAX];
> +

Whitespace error here.

> + desc = XkbAllocKeyboard();
> + if (!desc) {

This can be put into one if clause, no need for two lines.

Additionally, can you send one patch instead of a series regarding your
"Free strduped string"?

Cheers!
Aaron

-- 
Web: https://drkhsh.at/ or http://drkhsh5rv6pnahas.onion/
Gopher: gopher://drkhsh.at or gopher://drkhsh5rv6pnahas.onion
GPG: 0x7A65E38D55BE96FE
Fingerprint: 4688 907C 8720 3318 0D9F AFDE 7A65 E38D 55BE 96FE
Received on Wed May 23 2018 - 00:29:33 CEST

This archive was generated by hypermail 2.3.0 : Wed May 23 2018 - 00:36:23 CEST