Restore a mistakenly removed goto on error in js_build_module_ns()

[igorburago]

The goto in question was accidentally removed in 159fe28 as part of the bug fix for #329.

[bellard]

applied

[igorburago]

Thanks.

Leaving the braces in for the body of an if statement with a heavily indented condition would have helped for a similar mistake to not slip through the cracks again — and it wouldn't violate the codebase's style, as there are plenty of ifs with braced single-statement bodies, including those where it's just a single return or a goto, as well — but that's up to you, of course.

[bellard]

Yes I hesitated and perhaps I should have kept them.

[chqrlie]

This if should definitely have braces. The (unwritten) rule is simple: braces for if/else, for and while statements can be omitted only for very simple cases where each part is a single line. Since the test spans 4 lines, the braces are in. We should probably add a short style guide in the repository.

[igorburago]

Out of curiosity, I scanned the codebase for statements like that, and as far as I can gather, there are only 147 of them in quickjs.c and quickjs-libc.c: 143 ifs with a single-statement body where either the condition or the body spans multiple lines, as well as 3 elses and 1 while (no fors) that control multiline statements. There are 4 more ifs like that in libregexp.c and libunicode.c, as well as 16 ifs and 2 whiles in run-test262.c and unicode_gen.c.

Here is the list of all those occurrences: stmts-to-brace-list.txt. For reproducibility: the list was obtained by running the following command in the root directory of the repository. (Apologies for the obtuse Vim sorcery; it happened to be the most convenient tool at hand.)

vim -u NONE -n -Es <<'ENDVIM'
  let cond_or_stmt  = '^\(\s\+\)\%(if\|for\|while\)\s*(.\+\%()\s*{\s*\%(\/\*.*\*\/\s*\)\?\)\@<!\n'
  let cond_or_stmt .= '\%(\1\s\+\%(\%(if\|for\|while\)\s*(\)\@!\S.\+\%()\s*{\s*\%(\/\*.*\*\/\s*\)\?\)\@<!\n\)\+'
  let cond_or_stmt .= '\%(\1\s\+\%(\%(if\|for\|while\)\s*(\)\@!\S.\+\%()\s*{\s*\%(\/\*.*\*\/\s*\)\?\|;\s*\)\@<!\n\)*'
  let cond_or_stmt .= '\1\s\+.\+;\s*$'
  let else_stmt  = '^\s\+}\?\s*else\s*\n'
  let else_stmt .= '\s\+\%(#\|if\s*(\)\@!\S.\+\%(;\s*\%(\/\*.*\*\/\s*\)\?\)\@<!$'
  let @/ = cond_or_stmt . '\|' . else_stmt
  vimgrep // *.c
  copen | set ma
  %s/|\(\d\+\)[^|]\+|/:\1:/
  w! stmts-to-brace-list.txt
  qa!
ENDVIM

Since I now have these locations on file, I can easily make a pull request for the missing curly brackets in those, if you'd like. Even though it's only a minor cleanup, perhaps it's worthwhile, if only to make bugs like this one less likely to reoccur.