GNU bug report logs - #58855
[PATCH 0/5] Update mcron to latest commit

Previous Next

Full log

Message #25 received at 58855 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: guix-devel <at> gnu.org, 58855 <at> debbugs.gnu.org
Subject: Layout of ‘define-configuration’ records
Date: Thu, 17 Nov 2022 23:37:49 +0100

Hi,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> This is so that the first field of the generated record matches the first one
> declared, which makes 'define-configuration' record API compatible with
> define-record-type* ones.
>
> * gnu/services/configuration.scm (define-configuration-helper): Move the
> %location field below the ones declared by the user.
> * gnu/services/monitoring.scm (zabbix-front-end-config): Adjust match pattern
> accordingly.

[...]

> +++ b/gnu/services/configuration.scm
> @@ -242,17 +242,17 @@ (define-record-type* #,(id #'stem #'< #'stem #'>)
>                 stem
>                 #,(id #'stem #'make- #'stem)
>                 #,(id #'stem #'stem #'?)
> -               (%location #,(id #'stem #'stem #'-location)
> -                          (default (and=> (current-source-location)
> -                                          source-properties->location))
> -                          (innate))
>                 #,@(map (lambda (name getter def)
>                           #`(#,name #,getter (default #,def)
>                                     (sanitize
>                                      #,(id #'stem #'validate- #'stem #'- name))))
>                         #'(field ...)
>                         #'(field-getter ...)
> -                       #'(field-default ...)))
> +                       #'(field-default ...))
> +               (%location #,(id #'stem #'stem #'-location)
> +                          (default (and=> (current-source-location)
> +                                          source-properties->location))
> +                          (innate)))

Moving the field last is problematic as we’ve seen for any user that
uses ‘match’ on records—something that’s not recommended but still used
a lot.

>  (define (zabbix-front-end-config config)
>    (match-record config <zabbix-front-end-configuration>
> -    (%location db-host db-port db-name db-user db-password db-secret-file
> -               zabbix-host zabbix-port)
> +    (db-host db-port db-name db-user db-password db-secret-file
> +             zabbix-host zabbix-port %location)

This change has no effect because ‘match-record’ matches fields by name,
precisely to avoid problems when changing the layout of record types.

I’m not sure what was meant by “compatible” in the commit log; how
fields are laid out is something user code should not be aware of.

The only thing to keep in mind is that records must not be matched with
‘match’ because then the code silently breaks when the record type
layout is changed.  This is why ‘match-record’ was introduced:

  https://issues.guix.gnu.org/28960#4

One last thing: placing ‘%location’ first can let us implement:

  (define (configuration-location config)
    (struct-ref config 0))

As if there were type inheritance.  I didn’t see any indication that
this is actually done anywhere, but it could have been the case (it’s
not an unusual pattern) and it’s still something we might want to do.

Does that make sense?

Thanks,
Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.