Re: [PATCH v2 3/8] qnum: QNumValue type for QNum value literals

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Nov 24, 2020 at 04:20:37PM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Tue, Nov 24, 2020 at 09:49:30AM +0100, Markus Armbruster wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >> 
>> >> > On Mon, Nov 23, 2020 at 08:51:27AM +0100, Markus Armbruster wrote:
>> >> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >> >> 
>> >> >> > On Fri, Nov 20, 2020 at 06:29:16AM +0100, Markus Armbruster wrote:
>> >> >> [...]
>> >> >> >> When the structure of a data type is to be kept away from its 
>> >> >> >> users, I
>> >> >> >> prefer to keep it out of the public header, so the compiler 
>> >> >> >> enforces the
>> >> >> >> encapsulation.
>> >> >> >
>> >> >> > I prefer that too, except that it is impossible when users of the
>> >> >> > API need the compiler to know the struct size.
>> >> >> 
>> >> >> There are cases where the structure of a data type should be
>> >> >> encapsulated, yet its size must be made known for performance (avoid
>> >> >> dynamic memory allocation and pointer chasing).
>> >> >> 
>> >> >> Need for encapsulation correlates with complex algorithms and data
>> >> >> structures.  The cost of dynamic allocation is often in the noise then.
>> >> >
>> >> > I don't know what we are talking about anymore.  None of this
>> >> > applies to the QNum API, right?
>> >> >
>> >> > QNum/QNumValue are not complex data structures, and the reason we
>> >> > need the compiler to know the size of QNumValue is not related to
>> >> > performance at all.
>> >> 
>> >> We started with the question whether to make QNumValue's members
>> >> private.  We digressed to the question when to make members private.
>> >> So back to the original question.
>> >> 
>> >> > We might still want to discourage users of the QNum API from
>> >> > accessing QNum.u/QNumValue.u directly.  Documenting the field as
>> >> > private is a very easy way to do it.
>> >> 
>> >> It's a complete non-issue.  QNum has been around for years, and we
>> >> haven't had any issues that could've been plausibly avoided by asking
>> >> people to refrain from accessing its members.
>> >> 
>> >> If there was an actual need to keep the members private, I'd move the
>> >> struct out of the header, so the compiler enforces privacy.
>> >
>> > Understood.  There's still a question I'd like to answer, to
>> > decide how the API documentation should look like:
>> >
>> >   Is QNum.u/QNumValue.u required to be part of the API
>> >   documentation?
>> >
>> > If accessing that field directly is not necessary for using the
>> > API, I don't think it should appear in the documentation (because
>> > it would be just noise).
>> 
>> The current patch's comment on QNumValue looks good to me.
>> 
>> Does this answer your question?
>
> The current patch (v3) doesn't address the question.  It doesn't
> include documentation for the field, but doesn't hide it.
> kernel-doc will print a warning on that case.

Do we care?  How many such warnings exist before the patch?  Does this
series add just this one, or more?

Use your judgement, then be ready to explain it :)