Teng Long <dyroneteng@xxxxxxxxx> writes:
> A simple refactoring was done to the "show_tree" function, intead by
> using bitwise operations to recognize the format for printing to
> stdout. The reason for doing this is that we don't want to increase
> the readability difficulty with the addition of "-object-only",
> making this part of the logic easier to read and expand.
The resulting code looks unnecessarily complex and brittle; some
SHOW_FOO mean SHOW_FOO_ONLY_AND_NOTHING_ELSE while other SHOW_BAR
means SHOW_BAR_BUT_WE_MAY_SHOW_OTHER_THINGS_IN_LATER_PART, and the
distinction is not clear from their names (which means it is hard
to later extend and enhance the behaviour of the code).
> + if (!(shown_bits ^ SHOW_FILE_NAME)) {
Is the use of XOR operator significant here?
I.e. "if (shown_bits & SHOW_FILE_NAME)" would have been a much more
natural way to guard "this is a block that shows the file name",
than "the result MUST BE all bits off if we flip SHOW_FILE_NAME bit
off". If various SHOW_FOO bits are meant to be mutually exclusive,
then "if ((shown_bits & SHOW_FILE_NAME) == SHOW_FILE_NAME)" would
also make sense, but as I said upfront, it is unclear to me if
shown_bits are meant to be a collection of "this bit means this
field is shown (and it implies nothing else)", so I dunno.
