behave-screen-edge code is a mess (cmd_behave_screen_edge)

[Lekensteyn]

While fixing bugs found with Clang static analyzer, I encountered the cmd_behave_screen_edge function.

Issues that make me avoid fixing this code:

• printf debugging prints
• duplication of timer code with
• no comments describing what the timer code is used for.

The actual issue was a "Dead assignment" warning, in cmd_behave_screen_edge.c:295. Variable trigger is set to False every time the loop is executed.

Another question: why is need_new_context = True written on line 294? context_execute does not seem to free tmpcontext. Perhaps the tmpcontext = calloc(..) part should be moved outside the while loop and the copy be made unconditional? I don't know... I am not using this option and I am not going to fix it either.

[jordansissel]

I'll buy the 'code is a mess' argument given , but I'm curious - what problem are you encountering? I'm not sure how best to answer your question without knowing your problem, so before I answer any code linting/analysis questions, let's figure out what that is! :)

[jordansissel]

To be honest, having reviewed this code briefly just now, it looks like I refactored hastily this code a few times (which explains the end-of-loop 'dead assignments' and such), but again, let's get to what your problem is so that we may solve it :)

[Lekensteyn]

I was having issues with xdotool type --window $ID somekeystore, it would be entered in the current window instead of the window selected by $ID (actually, I tried searchwindow --name Firefox).

Our of curiousity, I ran scan-build on this project which found over 30 potentional bugs. I am in process of fixing them, you can expect a pull request tomorrow for that.

[jordansissel]

I rely mostly on the test suite to find bugs; any chance you can add tests for any bugs you're fixing that are indicated by your tools?

[Lekensteyn]

I will try to add tests where applicable. Some of the bugs occur when feeding unexpected input, or rely on undefined behavior.

Aside, what do you think of using the goto keyword?

This:

  void *p1 = NULL;
  void *p2 = NULL;

  if (cond) {
    return EXIT_FAILURE;
  }

  p1 = malloc(...);

  if (condition) {
    /* note: leak of p1 */
    return EXIT_FAILURE;
  }

...

  free(p1);
  free(p2);

  return EXIT_SUCCESS;

could be converted to:

  int ret = EXIT_FAILURE;
  void *p1 = NULL;
  void *p2 = NULL;

  if (cond) {
    return EXIT_FAILURE;
  }

  p1 = malloc(...);

  if (condition) {
    goto free_p1;
  }

...

  ret = EXIT_SUCCESS;
free_p2:
  free(p2);
free_p1:
  free(p1);
  return ret;

(real example: cmd_exec in cmd_exec.c, p1 is terminator and p2 is command)

Btw, how to test for crashes? Undefined behavior is difficult to test...

[jordansissel]

Regarding typing to firefox; I just tested again now with Firefox 25 it seems to work:

% xdotool getactivewindow 
37748739
% xdotool search --classname Navigator 
56623242
% xdotool search --classname Navigator   key ctrl+t
# result: firefox opens a new tab

[jordansissel]

re: using goto for cleanup like like a finally(java)/ensure(ruby)/defer(go) +1 I would approve this refactoring.

[Lekensteyn]

key is working, but type isn't:

xdotool search --name behave type x

I have pushed the fixes I have done so far here: https://github.com/Lekensteyn/xdotool/compare/clang-fixes

There are still 18 memleaks, 1 dead assignment and 11 logic errors (garbage/undefined) to fix (including the ones from this issue).

[jordansissel]

it'd be nice if there were a more straight forward way to add cleanup tasks though, instead of having multiple goto labels, just queue up some function calls (like 'defer' in go) and call them all in order at the end.

Interested in implementing that? Shouldn't be too hard ;)

[Lekensteyn]

For just freeing up resources one label is enough (free(NULL)) is fine, that should not need special treatment.

[jordansissel]

key is working, but type isn't

'type' defaults to --window 0, I believe, incorrectly. This works:

% xdotool search --classname Navigator   type --window "%1" 'hello world'

cmd_key.c has:

    /* use %1 if there is a window stack */
   if (window_arg == NULL && context->nwindows > 0) {
     window_arg = "%1";
   }

And cmd_type does not. cmd_type should also do this "default to %1 if window_arg is not specified and there is a window stack"

[jordansissel]

Also, I very much appreciate your energy put into helping improve this project :)

[Lekensteyn]

Thanks for the tool, it already saved me some time as a workaround for bugs :-)

What do you think of the pattern I used here: https://github.com/Lekensteyn/xdotool/commit/38cd31bef552165d82f846a9bd61138c0d125475

I could also have introduced some macros, but I feel that this is simple enough to avoid macros.

[jordansissel]

38cd31b looks good to me!

[jordansissel]

Agreed re: macros; it's simple enough.