Re: [hackers] [dwm][PATCH] cleanup, saves 7 LOC

2019-12-24 Thread Quentin Rameau 
> As of [0], the Xorg man page for XFree reads:
> 
> "If data is NULL, no operation is performed."
> 
> It was changed to reflect the behavior that Xorg has always had.
> XFree() has been #defined to free() since their initial code import in
> 2003.  However, it looks like other documentation was not updated, and
> it's understandable if you want to keep the NULL checks.
> 
> [0]: https://gitlab.freedesktop.org/xorg/lib/libx11/commit/78851f6

Agreed.

> > I think the current sendevent() code is more readable.

Agreed too.

Please provide both changes as separate commits, one for XFree()
unconditional call, and another one for refactoring sendevent().

Thanks!

Re: [hackers] [dwm][PATCH] cleanup, saves 7 LOC

2019-12-23 Thread Devin J. Pohly 
On Mon, Dec 23, 2019 at 11:21:25AM +0100, Hiltjo Posthuma wrote:
> Hi,
> 
> Regarding XFree():
> https://www.x.org/releases/current/doc/libX11/libX11/libX11.txt
> 
> "A NULL pointer cannot be passed to this function."
> 
> Where is it specified it can be NULL?

As of [0], the Xorg man page for XFree reads:

"If data is NULL, no operation is performed."

It was changed to reflect the behavior that Xorg has always had.
XFree() has been #defined to free() since their initial code import in
2003.  However, it looks like other documentation was not updated, and
it's understandable if you want to keep the NULL checks.

[0]: https://gitlab.freedesktop.org/xorg/lib/libx11/commit/78851f6


> I think the current sendevent() code is more readable.

Okay.  Thanks for taking a look at it.

> 
> -- 
> Kind regards,
> Hiltjo
> 

-- 
<><

Re: [hackers] [dwm][PATCH] cleanup, saves 7 LOC

2019-12-23 Thread Hiltjo Posthuma 
On Sun, Dec 22, 2019 at 11:27:16AM -0600, Devin J. Pohly wrote:
> Not too aggressive, stuck to changes that should be uncontroversial:
> - XFree, like free, is specified to do nothing when passed NULL, so no
>   need to duplicate that check ourselves
> - Flip if conditions to save lines in enternotify and maprequest
> - No need for the "exists" variable in sendevent when "n" will suffice
> ---
>  dwm.c | 50 ++
>  1 file changed, 22 insertions(+), 28 deletions(-)
> 
> diff --git a/dwm.c b/dwm.c
> index 4465af1..7fa5055 100644
> --- a/dwm.c
> +++ b/dwm.c
> @@ -304,10 +304,8 @@ applyrules(Client *c)
>   c->mon = m;
>   }
>   }
> - if (ch.res_class)
> - XFree(ch.res_class);
> - if (ch.res_name)
> - XFree(ch.res_name);
> + XFree(ch.res_class);
> + XFree(ch.res_name);
>   c->tags = c->tags & TAGMASK ? c->tags & TAGMASK : 
> c->mon->tagset[c->mon->seltags];
>  }
>  
> @@ -765,9 +763,8 @@ enternotify(XEvent *e)
>   if (m != selmon) {
>   unfocus(selmon->sel, 1);
>   selmon = m;
> - } else if (!c || c == selmon->sel)
> - return;
> - focus(c);
> + } else if (c && c != selmon->sel)
> + focus(c);
>  }
>  
>  void
> @@ -1094,10 +1091,9 @@ maprequest(XEvent *e)
>  
>   if (!XGetWindowAttributes(dpy, ev->window, &wa))
>   return;
> - if (wa.override_redirect)
> + if (wa.override_redirect || wintoclient(ev->window))
>   return;
> - if (!wintoclient(ev->window))
> - manage(ev->window, &wa);
> + manage(ev->window, &wa);
>  }
>  
>  void
> @@ -1402,8 +1398,7 @@ scan(void)
>   && (wa.map_state == IsViewable || getstate(wins[i]) == 
> IconicState))
>   manage(wins[i], &wa);
>   }
> - if (wins)
> - XFree(wins);
> + XFree(wins);
>   }
>  }
>  
> @@ -1437,24 +1432,23 @@ sendevent(Client *c, Atom proto)
>  {
>   int n;
>   Atom *protocols;
> - int exists = 0;
>   XEvent ev;
>  
> - if (XGetWMProtocols(dpy, c->win, &protocols, &n)) {
> - while (!exists && n--)
> - exists = protocols[n] == proto;
> - XFree(protocols);
> - }
> - if (exists) {
> - ev.type = ClientMessage;
> - ev.xclient.window = c->win;
> - ev.xclient.message_type = wmatom[WMProtocols];
> - ev.xclient.format = 32;
> - ev.xclient.data.l[0] = proto;
> - ev.xclient.data.l[1] = CurrentTime;
> - XSendEvent(dpy, c->win, False, NoEventMask, &ev);
> - }
> - return exists;
> + if (!XGetWMProtocols(dpy, c->win, &protocols, &n))
> + return 0;
> + while (n-- && protocols[n] != proto);
> + XFree(protocols);
> + if (n < 0)
> + return 0;
> +
> + ev.type = ClientMessage;
> + ev.xclient.window = c->win;
> + ev.xclient.message_type = wmatom[WMProtocols];
> + ev.xclient.format = 32;
> + ev.xclient.data.l[0] = proto;
> + ev.xclient.data.l[1] = CurrentTime;
> + XSendEvent(dpy, c->win, False, NoEventMask, &ev);
> + return 1;
>  }
>  
>  void
> -- 
> 2.24.1
> 
> 

Hi,

Regarding XFree():
https://www.x.org/releases/current/doc/libX11/libX11/libX11.txt

"A NULL pointer cannot be passed to this function."

Where is it specified it can be NULL?


I think the current sendevent() code is more readable.

-- 
Kind regards,
Hiltjo

[hackers] [dwm][PATCH] cleanup, saves 7 LOC

2019-12-22 Thread Devin J. Pohly 
Not too aggressive, stuck to changes that should be uncontroversial:
- XFree, like free, is specified to do nothing when passed NULL, so no
  need to duplicate that check ourselves
- Flip if conditions to save lines in enternotify and maprequest
- No need for the "exists" variable in sendevent when "n" will suffice
---
 dwm.c | 50 ++
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/dwm.c b/dwm.c
index 4465af1..7fa5055 100644
--- a/dwm.c
+++ b/dwm.c
@@ -304,10 +304,8 @@ applyrules(Client *c)
c->mon = m;
}
}
-   if (ch.res_class)
-   XFree(ch.res_class);
-   if (ch.res_name)
-   XFree(ch.res_name);
+   XFree(ch.res_class);
+   XFree(ch.res_name);
c->tags = c->tags & TAGMASK ? c->tags & TAGMASK : 
c->mon->tagset[c->mon->seltags];
 }
 
@@ -765,9 +763,8 @@ enternotify(XEvent *e)
if (m != selmon) {
unfocus(selmon->sel, 1);
selmon = m;
-   } else if (!c || c == selmon->sel)
-   return;
-   focus(c);
+   } else if (c && c != selmon->sel)
+   focus(c);
 }
 
 void
@@ -1094,10 +1091,9 @@ maprequest(XEvent *e)
 
if (!XGetWindowAttributes(dpy, ev->window, &wa))
return;
-   if (wa.override_redirect)
+   if (wa.override_redirect || wintoclient(ev->window))
return;
-   if (!wintoclient(ev->window))
-   manage(ev->window, &wa);
+   manage(ev->window, &wa);
 }
 
 void
@@ -1402,8 +1398,7 @@ scan(void)
&& (wa.map_state == IsViewable || getstate(wins[i]) == 
IconicState))
manage(wins[i], &wa);
}
-   if (wins)
-   XFree(wins);
+   XFree(wins);
}
 }
 
@@ -1437,24 +1432,23 @@ sendevent(Client *c, Atom proto)
 {
int n;
Atom *protocols;
-   int exists = 0;
XEvent ev;
 
-   if (XGetWMProtocols(dpy, c->win, &protocols, &n)) {
-   while (!exists && n--)
-   exists = protocols[n] == proto;
-   XFree(protocols);
-   }
-   if (exists) {
-   ev.type = ClientMessage;
-   ev.xclient.window = c->win;
-   ev.xclient.message_type = wmatom[WMProtocols];
-   ev.xclient.format = 32;
-   ev.xclient.data.l[0] = proto;
-   ev.xclient.data.l[1] = CurrentTime;
-   XSendEvent(dpy, c->win, False, NoEventMask, &ev);
-   }
-   return exists;
+   if (!XGetWMProtocols(dpy, c->win, &protocols, &n))
+   return 0;
+   while (n-- && protocols[n] != proto);
+   XFree(protocols);
+   if (n < 0)
+   return 0;
+
+   ev.type = ClientMessage;
+   ev.xclient.window = c->win;
+   ev.xclient.message_type = wmatom[WMProtocols];
+   ev.xclient.format = 32;
+   ev.xclient.data.l[0] = proto;
+   ev.xclient.data.l[1] = CurrentTime;
+   XSendEvent(dpy, c->win, False, NoEventMask, &ev);
+   return 1;
 }
 
 void
-- 
2.24.1