Re: encapsulating code properties

[Paul Eggert]

"Joel E. Denny" <address@hidden> writes:

> The following patch (committed) creates a code_props structure and 
> interface that (for now) encapsulates the first three fields above and 
> functionality related to them.

I'm afraid I'm going to have to object to this one.  The basic idea is
OK, but there are too many changes here, and I can't say I agree with
them all.  The changes slow Bison down in ways that I can measure, and
I don't see why they have to be written the way they are.

How about if we back out this patch and then talk about the changes
one by one?

For starters:

The patch changes Bison to pass too many copies of structures around,
when we should be passing references to structures.  I suspect this is
what hurts Bison's performance.

Code like this is unnecessarily verbose:

        rules[ruleno].action_location =
          code_props_location_get (p->action_props);

It's easier to read this:

        rules[ruleno].action_location = p->action_props->location;

If we turn every '->' into a foo_bar_baz_boof_get, our code will get
harder to read, with little real benefit.  I know why you encapsulated
the members inside getters; but to my mind the cost exceeds the
benefit, simply in terms of reading the code.

The Doxygenated comments are hard to read and often don't offer much.
For example:

  /**
   * \brief
   *   The value returned by \c code_props_is_value_used for this
   *   \c code_props.
   */
  bool is_value_used;

This comment (a) doesn't tell me anything useful, and (b) is hard to
read because of those annoying "\c"s.  How about if we get rid of all
the \c occurrences, for starters?  And how about if we get rid of
those getter methods, and just refer to the members directly?