On 4/16/20 11:12 AM, Alvaro Herrera wrote: > On 2020-Apr-16, Hamlin, Garick L wrote: > >> With the old expression 'something;' would be stripped away. >> Is that an issue where this this is used? Why are we parsing >> these headers? > These are files from which bootstrap catalog data is generated, which is > why we parse from Perl; but also where C structs are declared, which is > why they're C. > > I think switching to non-greedy is a win in itself. Non-capturing > parens is probably a wash (this doesn't run often so the performance > argument isn't very interesting).
Yeah, I'm inclined to fix this independently of the perlcritic stuff. The change is more readable and more correct as well as being perlcritic friendly. I might take a closer look at Catalog.pm. Meanwhile, the other regex highlighted in the patch, in Solution.pm: if (/^AC_INIT\(\[([^\]]+)\], \[([^\]]+)\], \[([^\]]+)\], \[([^\]]*)\], \[([^\]]+)\]/) is sufficiently horrid that I think we should see if we can rewrite it, maybe as an extended regex. And a better fix here instead of marking the fourth group as non-capturing would be simply to get rid of the parens altogether. The serve no purpose at all. > > An example. This eval in Catalog.pm > > + ## no critic (ProhibitStringyEval) > + ## no critic (RequireCheckingReturnValueOfEval) > + eval '$hash_ref = ' . $_; > > is really weird stuff generally speaking, and the fact that we have to > mark it specially for critic is a good indicator of that -- it serves as > documentation. Catalog.pm is all a huge weird hack, but it's a critically > important hack. Heck, what about RequireCheckingReturnValueOfEval -- > should we instead consider actually checking the return value of eval? > It would seem to make sense, would it not? (Not for this patch, though > -- I would be fine with just adding the nocritic line now, and removing > it later while fixing that). +1 > > All in all, I think it's a positive value in having this code be checked > with a bit more strength -- checks that are pointless in, say, t/00*.pl > prove files. thanks cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
