regcomp.c: Split a too-large-function into two #18835
Conversation
| POSIXL + invert * (NPOSIXL - POSIXL)); | ||
| FLAGS(REGNODE_p(ret)) = classnum; | ||
| goto not_anyof; | ||
| op = POSIXL + *invert * (NPOSIXL - POSIXL); |
There was a problem hiding this comment.
Is there any reason not to use the more obvious ternary here: *invert ? NPOSIXL : POSIXL? (Maybe for a future commit.)
There was a problem hiding this comment.
Yes, if this is done, It shouldn't be in this commit, which is trying to avoid doing more than necessary in splitting the function.
Your proposal is clearer. A reason not to do what you suggest is because this is a repeat of a common paradigm in this file, so it isn't out of the blue, and it works without change if a new type of POSIXL regnode (and inverse) is added, which has long been a possibility and remains so.
My attempt to insulate from the leading tab removal the year-old commits finally pushed as 77a6d54 and 403d7eb failed miserably. I think it is some bug in git. Seemingly random groups of lines were indented differently than adjacent ones. Anyway, I spent a bunch of time sorting it all out, and this is the result. This commit includes the comment change suggestions from @hvds in PR Perl#18835.
My attempt to insulate from the leading tab removal the year-old commits finally pushed as 77a6d54 and 403d7eb failed miserably. I think it is some bug in git. Seemingly random groups of lines were indented differently than adjacent ones. Anyway, I spent a bunch of time sorting it all out, and this is the result. This commit includes the comment change suggestions from @hvds in PR Perl#18835.
|
@khwilliamson I suggest it would be better to separate the whitespace fixup from the comment changes; I've created hv/khwpr to demonstrate that, in case it is helpful. |
My attempt to insulate from the leading tab removal the year-old commits finally pushed as 77a6d54 and 403d7eb failed miserably. I think it is some bug in git. Seemingly random groups of lines were indented differently than adjacent ones. Anyway, I spent a bunch of time sorting it all out, and this is the result.
Comment change suggestions from @hvds in PR Perl#18835.
Based on a comment from @hvds, I think it better if this function return an impossible node value if it didn't find a node to use.
This code is irrelevant unless the condition of the block immediately before it is TRUE, so move it to within that block.
This variable will be used in future commits in more places, so compute it just once.
to silence some compiler's that were warning
Comment change suggestions from @hvds in PR Perl#18835.
|
@khwilliamson, earlier in the discussion in this p.r. you appear to have been having second thoughts about some of the code. Given that (and given merge conflicts), would it be better to close out this p.r. and start a fresh one? Thank you very much. |
S_regclass() is unwieldy. This commit splits it into two nearly equal
size parts. More could be done.