Davem/refactor warnings pl #19667
Davem/refactor warnings pl #19667
Conversation
There was a problem hiding this comment.
All looks good to me. In commit messages:
s/orderValues()/valueWalk()/ in "eliminate broken dup check"
s/on;y/only/ in "eliminate global %v_list var"
Now that %tree has been renamed, it would also be nice to rename the various in-function $tre variables to $tree.
There was a problem hiding this comment.
LGTM
I'm in two minds about whether to merge it before 5.36 or not. As you observe, it's only used as part of the regen process when we add a new warning, which we're not going to do before 5.36. So on the one hand adding it now won't affect anything, but on the other hand there's no urgent need to.
We could merge it now, or apply the "defer-next-dev" tag to remind us to do it as part of 5.37 startup.
|
On Thu, Apr 28, 2022 at 05:51:12AM -0700, Paul Evans wrote:
@leonerd approved this pull request.
LGTM
I'm in two minds about whether to merge it before 5.36 or not. As you
observe, it's only used as part of the regen process when we add a new
warning, which we're not going to do before 5.36. So on the one hand
adding it now won't affect anything, but on the other hand there's no
urgent need to.
I forgot to mention that I want to do some moderate-scale reworking of
warnings.pm and its pod (starting now, for release after 5.36) which
depends on the warnings.pl changes, so having that merged now makes it
easier for me to maintain my private warnings branch before 5.36.
…--
The Enterprise is involved in a bizarre time-warp experience which is in
some way unconnected with the Late 20th Century.
-- Things That Never Happen in "Star Trek" #14
|
Move a couple of large chunks of <<'EOF' code text to the end of the file (but before __END__) to make the flow of code easier to see. The output generated is unchanged.
$offset was added in 2000, but hasn't actually been used for a long while
A block of code no longer declares any lexical variables, so remove the
block. Apart from removing a '{' and '}', this is a whitespace-only
change.
various lexical vars like $tree, $def are populated early on, then their values are used later in many places, including directly in subs. Rename these vars to be uppercase and with more meaningful names, to emphasise their globalness. (Ideally they really ought to be local and passed as arguments to all the subs that use them, but that's more work.)
This doesn't seem to be used any more
This lexical had global scope. Instead, make it a parameter to sub valueWalk(), since it's only used temporarily by two subs.
valueWalk() checks for a duplicate warnings name. However, 1) This is also done in walk(), so is redundant. 2) It is broken. Originally it declared @list but checked %list; %list is global and was renamed to %CATEGORIES to make it clear it was a global (and thus unrelated to @list). So its probably never worked.
rename my($warn, $pm) to ($warn_h, $warn_pm) to make it easier to see that they're the filehandles for warnings.h and warnings.pm
Explain this structure, which is the input used to define all the warnings.
Since the global variable $tree was renamed $TREE, we can now rename all the instances of a sub parameter $tre to '$tree'.
iabyn
force-pushed
the
davem/refactor_warnings_pl
branch
from
9f1f50d to
e2f858e
Compare
|
rebased, fixed the commit msgs and s/tre/tree as suggested by Hugo |
+1 |
|
I've now merged this. I'm not going to be very available for the next week or so, so if it causes any issues, feel free to revert it. |
Refactor regen/warnings.pl. The src for this tool was a mess: it started out 20 years ago as a small utility, then grew. Lots of badly-named global variables, no comments about what subs do etc. This series of commits cleans things up, refactors, adds comments etc. In theory it makes no functional changes, and indeed as run currently it generates exactly the same warnings.h and lib/warning.pm files as before.
I think this can be safely merged before 5.36.0. It's a tool that's only used when developing the perl interpreter (and then only when adding a new warnings category), so even if were broken, it would make no different to an end user of 5.36.0.