On Mon, Jul 10, 2023 at 04:43:23PM +0200, Daniel Gustafsson wrote:
>> On 8 Jul 2023, at 23:43, Nathan Bossart <nathandbossart@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