> - More readable parameter definitions in params.c - workers/55022
> Only rename/refactor macros, no code change
Anyone have comments on this? It's a bit like a patch that cleans up
indentation in that it makes sense on its own but puts a crimp in
tracing changes from before it to after it.
It's true that if you use "blame", you will have to jump through one extra hoop but in my experience current code is read orders of magnitude more often than historical changes. Therefore, I would rather optimize the former than the latter.
> - The substitution flags (v!) are forbidden, also forbid (!v) - workers/54940
This one seems semantically OK but reading the diff leaves me
searching for something *else* that *ceases to be* an error. Did an
earlier patch change something related?
The error checks were added in
workers/51509. Afaict, their only goal is to prevent combining (!) with either (k) or (v). The code looks right on the surface. The "case '
k'" sets "h
keys = SCANPM_WANT
KEYS" and checks "(h
keys & ~SCANPM_WANT
KEYS)", so since the "case '
v'" sets "h
vals = SCANPM_WANT
VALS", it should check "(h
vals & ~SCANPM_WANT
VALS)", right? Except no. Since
"hvals" is only ever set SCANPM_WANTVALS, the current check never triggers. What we need is the same check as under "case 'k'".
> - Don't map ksh's ! to Zsh's (!k), map it to (k) - workers/55047
> Maybe controversial
That one says "DO NOT COMMIT" so I haven't considered doing so. The
only controversial bit for me is the part where ${!nameref} doesn't
act like ksh any more; I have no objection to ${!hash} or
${!hash[key]} working like (k) when namerefs aren't involved.
There are two patches in workers/55047. The "DO NOT COMMIT" only applies to the second one. This item is about the first one. I will reply in the thread to better explain why I think we need the first one.
> - Restrict named directories to scalar parameters set to a path - workers/54760
> Maybe controversial
I don't think I ever got an answer to my question at the end of
workers/54779? The thread veered down the path of replacing
workers/54759 with workers/54792
I also vaguely remember there were some questions/follow-ups. We can leave this one for later.
> The following two are rather simple but the first one has a trivial merge conflict (because a committed patch already added "for, " in an error message about loops).
>
> - Forbid break/continue in subshells - workers/55061
> - Forbid escaping past all enclosing loops in the same subshell - workers/55061
> Maybe controversial
I don't have a strong opinion on this ... I have the impression that
"break N" for a large N may have been used somewhere to escape from a
stack of nested loops, possibly to force a function call stack to
unwind, so I'm uncertain about making this case an error.
True, I remember at least thinking about using something like "break 999". If you do that in a function and then happen to call the function from within a $(…) that is itself in a loop, the break will trigger an error even though it would have performed exactly what was expected from it. On the other hand, if you try to use this trick to break out of nested loops of which some are in parent shells, the error message could be very useful to let you understand why your code isn't behaving as expected. Would a warning be a fair compromise? In the former case, you could use "break 999 2>/dev/null" to get rid of the unwanted message (although only if the function containing the "break 999" is under your control).
Philippe