On Saturday, April 16, 2016 01:25:09 PM Pedro Giffuni wrote:
> Hello;
> Using coccinelle, and some hand re-formatting, I generated a patch to
> make use of the nitems() macro in sys, which is too big for
> phabricator [1].
> I was careful to exclude anything from the contrib directory or
> any code that is shared with userland (as to not have to include
> additional headers).
> The patch is big but pretty safe, I think. The changes are small but
> still it touches many files[1].
> I would like some feedback on the convenience of doing such replacement.
> If it is a good idea we could do the same for roundup2() and, in fact,
> I think DragonFly has already done this.
> Regards,
> Pedro.
> [1] https://people.freebsd.org/~pfg/patches/sys-nitems.diff
> [2] For those too lazy to check [1], here is a list of affected files.
> M       sys/amd64/amd64/amd64_mem.c
> M       sys/amd64/amd64/machdep.c
> M       sys/amd64/linux/linux_sysvec.c
> M       sys/amd64/linux32/linux32_sysvec.c
> M       sys/arm/amlogic/aml8726/aml8726_clkmsr.c
> M       sys/arm/arm/db_interface.c
> M       sys/arm/at91/at91_pmc.c
> M       sys/arm/mv/armadaxp/armadaxp.c
> M       sys/arm/ti/cpsw/if_cpsw.c
> M       sys/arm/xscale/ixp425/ixp425.c
> M       sys/boot/common/part.c
> M       sys/boot/efi/loader/bootinfo.c
> M       sys/boot/mips/beri/boot2/boot2.c
> M       sys/boot/uboot/common/metadata.c
> M       sys/cam/ata/ata_da.c
> M       sys/cam/ata/ata_xpt.c

I would perhaps remove ata_quirk_table_size entirely and replace its
use with nitems() directly.  Probably best if that was a separate commit
though from the near-mechanical replacement.

> M       sys/cam/cam.c

Same here.  num_cam_status_entries is only used in one place and I think
directly using nitems() is probably clearer overall.

> M       sys/cam/scsi/scsi_all.c

Possibly the same here with sense_key_table_size.

> M       sys/cam/scsi/scsi_cd.c
> M       sys/cam/scsi/scsi_da.c
> M       sys/cam/scsi/scsi_sa.c
> M       sys/cam/scsi/scsi_xpt.c

Same as for ata_quirk_table_size (ironically this used the size in one
place and the expanded form of nitems in another while the ata variant
at least used the size in both places).

> M       sys/cam/scsi/smp_all.c
> M       sys/compat/linux/linux_socket.c
> M       sys/ddb/db_variables.c
> M       sys/dev/adb/adb_kbd.c
> M       sys/dev/advansys/adv_isa.c
> M       sys/dev/advansys/advlib.c
> M       sys/dev/advansys/adw_pci.c

Same here for num adw_num_pci_devs (only used once)

> M       sys/dev/advansys/adwlib.c

Probably the same here for adw_num_sync_rates (used twice).

> M       sys/dev/ae/if_ae.c

Same here for AE_DEVS_COUNT (only used once).

> M       sys/dev/age/if_age.c
> M       sys/dev/agp/agp.c
> M       sys/dev/agp/agp_ali.c
> M       sys/dev/agp/agp_amd64.c
> M       sys/dev/aha/aha_isa.c
> M       sys/dev/aic/aic.c
> M       sys/dev/aic/aic_cbus.c

Same here for AIC_ISA_NUMPORTS (only used once).

> M       sys/dev/aic/aic_isa.c

As above.

> M       sys/dev/ale/if_ale.c
> M       sys/dev/altera/atse/if_atse.c
> M       sys/dev/atkbdc/atkbd.c
> M       sys/dev/atkbdc/atkbdc.c
> M       sys/dev/atkbdc/psm.c
> M       sys/dev/bktr/bktr_core.c
> M       sys/dev/bwi/bwirf.c
> M       sys/dev/bwn/if_bwn.c
> M       sys/dev/cardbus/cardbus_cis.c
> M       sys/dev/digi/digi.c
> M       sys/dev/digi/digi_isa.c
> M       sys/dev/dwc/if_dwc.c
> M       sys/dev/ed/if_ed_hpp.c
> M       sys/dev/ed/if_ed_isa.c
> M       sys/dev/ed/if_ed_pci.c
> M       sys/dev/fb/creator.c

Same here for CREATOR_FB_MAP_SIZE (only used once).

> M       sys/dev/fb/fb.c
> M       sys/dev/fb/machfb.c
> M       sys/dev/fb/vesa.c
> M       sys/dev/fb/vga.c
> M       sys/dev/flash/mx25l.c
> M       sys/dev/hatm/if_hatm.c
> M       sys/dev/hifn/hifn7751.c
> M       sys/dev/hwpmc/hwpmc_amd.c

Same here for amd_event_codes_size (only used twice).

> M       sys/dev/hwpmc/hwpmc_core.c

Same here for niap_events.

> M       sys/dev/hwpmc/hwpmc_e500.c

e500_event_codes_size isn't even used.  It should just be removed.

> M       sys/dev/hwpmc/hwpmc_mips24k.c
> M       sys/dev/hwpmc/hwpmc_mips74k.c
> M       sys/dev/hwpmc/hwpmc_mpc7xxx.c

Same here for mpc7xxx_event_codes_size.

> M       sys/dev/hwpmc/hwpmc_octeon.c
> M       sys/dev/hwpmc/hwpmc_uncore.c

Same here for nucp_events.

> M       sys/dev/hwpmc/hwpmc_xscale.c

Same here for xscale_event_codes_size.

> M       sys/dev/if_ndis/if_ndis.c
> M       sys/dev/jme/if_jme.c
> M       sys/dev/kbd/kbd.c
> M       sys/dev/le/if_le_isa.c
> M       sys/dev/le/if_le_lebuffer.c

Same here for NLEMEDIA (only used once).

> M       sys/dev/le/if_le_ledma.c
> M       sys/dev/mlx/mlx.c
> M       sys/dev/mxge/if_mxge.c
> M       sys/dev/nand/nand_id.c
> M       sys/dev/ncr/ncr.c
> M       sys/dev/nctgpio/nctgpio.c
> M       sys/dev/nfe/if_nfe.c
> M       sys/dev/patm/if_patm_attach.c
> M       sys/dev/pccard/pccard_cis_quirks.c

Same here for n_pccard_cis_quirks (only used once).

> M       sys/dev/rc/rc.c
> M       sys/dev/re/if_re.c
> M       sys/dev/rl/if_rl.c
> M       sys/dev/rndtest/rndtest.c

Same here for RNDTEST_NTESTS (only used once).

> M       sys/dev/sf/if_sf.c
> M       sys/dev/sge/if_sge.c

Same here for 'cnt' (only used once).

> M       sys/dev/siba/siba.c

Same here for 'bound'.  I would actually simplify this function
even further so it just does 'return (sd)' instead of 'break'
in the loop body.  This means you can remove the '==' check
and just 'return (NULL)' if the loop completes.  It also means
that 'bound' is then only used once.

> M       sys/dev/sio/sio.c
> M       sys/dev/sound/isa/gusc.c
> M       sys/dev/sound/pci/emu10kx.c

Same here for 'n_cards' (it is only used once after each assignment).

> M       sys/dev/speaker/spkr.c
> M       sys/dev/stge/if_stge.c
> M       sys/dev/uart/uart_kbd_sun.c
> M       sys/dev/uart/uart_subr.c

Same here for 'uart_nclasses' (only used once).

> M       sys/dev/usb/input/ukbd.c
> M       sys/dev/usb/serial/u3g.c
> M       sys/dev/usb/serial/uchcom.c

Same here for NUM_DIVIDERS (only used once).

> M       sys/dev/usb/serial/umcs.c
> M       sys/dev/usb/serial/uplcom.c

Same here for N_UPLCOM_RATES (only used once).

> M       sys/dev/vkbd/vkbd.c
> M       sys/dev/wbwd/wbwd.c
> M       sys/fs/autofs/autofs.c
> M       sys/fs/nfs/nfs_commonkrpc.c
> M       sys/geom/part/g_part_bsd.c
> M       sys/geom/part/g_part_ebr.c
> M       sys/geom/part/g_part_ldm.c
> M       sys/geom/part/g_part_mbr.c
> M       sys/i386/i386/i686_mem.c
> M       sys/i386/i386/machdep.c
> M       sys/i386/ibcs2/ibcs2_sysvec.c
> M       sys/i386/linux/linux_sysvec.c
> M       sys/kern/kern_dump.c
> M       sys/kern/kern_ffclock.c
> M       sys/kern/kern_jail.c
> M       sys/kern/kern_ktrace.c
> M       sys/kern/subr_hash.c

Same here for NPRIMES (only used once).

> M       sys/kern/subr_witness.c

Same here for blessed_count (only used once).

> M       sys/kern/sysv_msg.c
> M       sys/kern/sysv_sem.c
> M       sys/kern/uipc_usrreq.c
> M       sys/mips/mips/db_interface.c
> M       sys/mips/nlm/board.c
> M       sys/mips/nlm/xlp_machdep.c

Same here for nreg (only used once).

> M       sys/mips/rmi/dev/nlge/if_nlge.c

Same here for n_gmac_buckers and n_regs (each only used once).

> M       sys/net/netisr.c

I would do the same here for netisr_dispatch_table_len.  It is
used in three loops, but in all three the code would be:

        for (i = 0; i < nitems(foo); i++) {
                /* use foo[i] */

For that idiom, I think using nitems is clearer to the reader vs one of
NFOO, nfoo, foo_size, foo_len, etc. if for no other reason than
consistency.  I think it is also more explicit.

> M       sys/net/rtsock.c
> M       sys/netgraph/atm/ng_atm.c
> M       sys/netgraph/bluetooth/socket/ng_btsocket.c

Same here for ng_btsocket_protosw_size (only used once).

> M       sys/netgraph/ng_gif_demux.c
> M       sys/netgraph/ng_iface.c

Possibly the same for NUM_FAMILIES in these two files.

> M       sys/netgraph/ng_socket.c
> M       sys/netinet/in_proto.c
> M       sys/netinet/tcp_syncache.c
> M       sys/netinet6/in6_proto.c
> M       sys/netipsec/key.c
> M       sys/netipsec/keysock.c
> M       sys/netnatm/natm_proto.c
> M       sys/netsmb/smb_smb.c

SMB_DIALECT_MAX isn't used and should just be removed.

> M       sys/nlm/nlm_prot_impl.c

Same here for version_count (only used once).

> M       sys/pc98/cbus/gdc.c
> M       sys/pc98/cbus/pckbd.c
> M       sys/pc98/cbus/scterm-sck.c
> M       sys/powerpc/powerpc/db_trace.c
> M       sys/powerpc/pseries/xics.c
> M       sys/security/audit/audit_bsm_klib.c

Same here for aue_open_count and aue_openat_count (each only used once).

> M       sys/security/audit/bsm_errno.c

Same here for bsm_errnos_count (used twice, but both in the for loop idiom).

> M       sys/security/audit/bsm_fcntl.c

Same here for bsm_fcntl_cmd_count (used twice, but both in the for loop

> M       sys/security/audit/bsm_socket_type.c

Same here for bsm_socket_types_count (used twice, but both in the for loop

> M       sys/sparc64/sparc64/db_trace.c
> M       sys/sparc64/sparc64/elf_machdep.c
> M       sys/sparc64/sparc64/trap.c
> M       sys/vm/vm_pager.c

Same here for npagers (only used once).

> M       sys/x86/isa/atpic.c
> M       sys/x86/x86/identcpu.c
> M       sys/x86/x86/local_apic.c

The changes overall look fine to me.  I just think we should take the chance
to inline cases where the variable is only ever used once or is only used in
the for loop idiom where I think the explicit nitems() is clearer to the

John Baldwin
freebsd-current@freebsd.org mailing list
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Reply via email to