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 idiom). > M sys/security/audit/bsm_socket_type.c Same here for bsm_socket_types_count (used twice, but both in the for loop idiom). > 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 reader. -- John BaldwinReceived on Mon Apr 18 2016 - 16:53:48 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:41:04 UTC