Web lists-archives.com

Re: [PATCH 01/19] revision.h: avoid bit fields in struct rev_info




On 5/8/2019 10:41 AM, Duy Nguyen wrote:
> On Wed, May 8, 2019 at 9:07 PM Derrick Stolee <stolee@xxxxxxxxx> wrote:
>>
>> On 5/8/2019 7:12 AM, Nguyễn Thái Ngọc Duy wrote:
>>> Bitfield addresses cannot be passed around in a pointer. This makes it
>>> hard to use parse-options to set/unset them. Turn this struct to
>>> normal integers. This of course increases the size of this struct
>>> multiple times, but since we only have a handful of rev_info variables
>>> around, memory consumption is not at all a concern.
>>
>> I think you are right that this memory trade-off shouldn't be a problem.
>>
>> What worries me instead is that we are using an "internal" data structure
>> for option parsing. Would it make more sense to create a struct for use
>> in the parse_opts method and a method that translates those options into
>> the bitfield in struct rev_info?
> 
> But we are doing that now (option parsing) using the same data
> structure. Why would changing from a custom parser to parse_options()
> affect what fields it should or should not touch in rev_info? Genuine
> question. Maybe you could elaborate more about "internal". I probably
> missed something. Or maybe this is a good opportunity to separate
> intermediate option parsing variables from the rest of rev_info?

You're right. I was unclear.

rev_info stores a lot of data. Some of the fields are important
in-memory structures that are crucial to the workings of revision.c
and are never used by builtin/rev-list.c. Combining this purpose
with the option parsing seems smelly to me.

Thinking more on it, I would prefer a more invasive change that may
pay off in the long term. These options, along with the "starting list"
values, could be extracted to a 'struct rev_options' that contains these
integers and the commit list. Then your option parsing changes could be
limited to a rev_options struct, which is later inserted into a rev_info
struct during setup_revisions().

Generally, the rev_info struct has too many members and could be split
into smaller pieces according to purpose. I created the topo_walk_info
struct as a way to not make the situation worse, but doesn't fix existing
pain.

My ramblings are mostly complaining about old code that grew organically
across many many quality additions. It is definitely hard to understand
the revision-walking code, and perhaps it would be easier to understand
with a little more structure.

The biggest issue with my suggestion is that it requires changing the
consumers of the options, as they would no longer live directly on the
rev_info struct. That would be a big change, even if it could be done
with string replacement.

Thanks,
-Stolee