Page MenuHomeFreeBSD

Always cast vop_F_args to vop_generic_args
Needs ReviewPublic

Authored by def on Nov 11 2022, 1:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 16 2022, 6:46 AM
Unknown Object (File)
Dec 14 2022, 3:21 AM
Subscribers

Details

Summary

Instead of using a vop_F_args.a_gen field, always cast a vop_F_args
object to vop_generic_args.

Before this change, there were 3 null_bypass() calls using
a vop_F_args.a_gen field and 5 null_bypass() calls using a cast to
vop_generic_args. This change makes all null_bypass() calls consistent.

Pointed out by: jrtc27

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 48299
Build 45185: arc lint + arc unit

Event Timeline

def requested review of this revision.Nov 11 2022, 1:05 PM

I believe all uses of &ap->a_gen are mine, and the casts are historic.
Isn't it more reliable to avoid cast and explicitly take the address of the generic member? Either case depends on the generic being the first member of the vop_arg struct for now.

Anyway, I am curious about your choice of the cast, Is it due to the calculated number of each case in the source?

This comment was removed by kib.
In D37359#849134, @kib wrote:

I believe all uses of &ap->a_gen are mine, and the casts are historic.
Isn't it more reliable to avoid cast and explicitly take the address of the generic member? Either case depends on the generic being the first member of the vop_arg struct for now.

Anyway, I am curious about your choice of the cast, Is it due to the calculated number of each case in the source?

I looked and the timestamps for use all over the place (interleaving). I slightly prefer the &gen version myself, but I think the cast was just used more times.

In D37359#849134, @kib wrote:

I believe all uses of &ap->a_gen are mine, and the casts are historic.
Isn't it more reliable to avoid cast and explicitly take the address of the generic member? Either case depends on the generic being the first member of the vop_arg struct for now.

Anyway, I am curious about your choice of the cast, Is it due to the calculated number of each case in the source?

My initial choice of the casts over the structure member was motivated by the code structure matching the intensions. An object is cast to a parent type of its structure type, rather than relies on a specific structure field that provides that functionality. Relying on the type system seemed like a more elegant solution to me.

On the other hand, one can also argue that explicitly using the structure member reflects the intensions as in this case a developer uses the field to indicate that they want to use the object as a parent structure object. However, every time the structure member name changes, all occurrences of the structure member must be updated. In case a parent structure name changes, all casts must be updated but it seems to be a justified cost since all function prototypes must be also updated in this case.

Having said that, I don't have a strong opinion what's the best choice here. If there's no strong argument for one of the choices, adapting the code to the most common scenario seems to be the right solution here.

My preference, as you could see from the earlier response, is to have the patch in reverse direction, using &a_gen instead of cast. Cast means that you fight the type system, not utilizing it.

Anyway, I do not intend to block either change, go ahead with something.