On Mon, Jul 10, 2023 at 04:43:23PM +0200, Daniel Gustafsson wrote: >> On 8 Jul 2023, at 23:43, Nathan Bossart <nathandboss...@gmail.com> wrote: >> Since "script" will be NULL and "db_used" will be false in the first >> iteration of the loop, couldn't we move this stuff to before the loop? > > We could. It's done this way to match how all the other checks are performing > the inner loop for consistency. I think being consistent is better than micro > optimizing in non-hot codepaths even though it adds some redundancy. > > [ ... ] > >> Won't "script" always be initialized here? If I'm following this code >> correctly, I think everything except the fclose() can be removed. > > You are right that this check is superfluous. This is again an artifact of > modelling the code around how the other checks work for consistency. At least > I think that's a good characteristic of the code.
I can't say I agree with this, but I'm not going to hold up the patch over it. FWIW I was looking at this more from a code simplification/readability standpoint. >> +static int n_data_types_usage_checks = 7; >> >> Can we determine this programmatically so that folks don't need to remember >> to update it? > > Fair point, I've added a counter loop to the beginning of the check function > to > calculate it. + /* Gather number of checks to perform */ + while (tmp->status != NULL) + n_data_types_usage_checks++; I think we need to tmp++ somewhere here. >> In fact, I wonder if we could just add the versions directly to >> data_types_usage_checks so that we don't need the separate hook functions. > > We could, but it would be sort of contrived I think since some check <= and > some == while some check the catversion as well (and new ones may have other > variants. I think this is the least paint-ourselves-in-a-corner version, if > we > feel it's needlessly complicated and no other variants are added we can > revisit > this. Makes sense. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
