Jessica Clarke <jrtc27@xxxxxxxxxx> writes: >> I actually wonder if it results in code that is much easier to >> follow if we did: >> >> * Introduce an "enum apply_symlink_treatment" that has >> APPLY_SYMLINK_GOES_AWAY and APPLY_SYMLINK_IN_RESULT as its >> possible values; >> >> * Make register_symlink_changes() and check_symlink_changes() >> work with "enum apply_symlink_treatment"; >> >> * (optional) stop using string_list() to store the symlink_changes; >> use strintmap and use strintmap_set() and strintmap_get() to >> access its entries, so that the ugly implementation detail >> (i.e. "the container type we use only has a (void *) field to >> store caller-supplied data, so we cast an integer and a pointer >> back and forth") can be safely hidden. > > Those would be better if you want a less-minimal change. The first two at least would make an understandable change, as opposed to the code as posted, which is totally opaque why we need to take size_t and cast it to uintptr_t in the first place. I have no confidence in myself that I would not accept a future patch that reverts the change made by this patch happily. I usually try to be careful to go back to the originating commit by running "blame" on the preimage and may find your commit, but that's only "usually". If I were to work only with the file contents after applying this patch, because no clue ... ent->util = (void *)((uintptr_t)what | ((uintptr_t)ent->util)); ... on this line of code hints why we must be called with size_t and have to cast it here, instead of working with uintptr_t throughout, I am reasonably sure I'd happily take such a patch and break your "fix" here. If we make the code pass around the enum, have a temporary variable of the same enum type to compute the next value we stuff in ent->util, and make an assignment of that enum value to ent->util, it is much less likely that I'd blindly accept a patch to take us back to deal with uintptr_t directly.
