On 04/18/16 13:27, John Baldwin wrote: > 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. > ... > > 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 > reader. > Huge thanks for the detailed review. I have done the cleanups related to consts and variables. I would prefer to leave the macros still there, even if they are used only once: in at least one case they actually help driver readability and may generally help understand the author's intent. It may also be that they help upstream (or downstream) maintainers. I am running a buildworld with the final nitems() changes. Regards, Pedro.Received on Thu Apr 21 2016 - 14:02:53 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:41:04 UTC