On Thu, Mar 06, 2025 at 01:35:52AM +0000, Benno Lossin wrote:
> On Thu Mar 6, 2025 at 2:29 AM CET, Danilo Krummrich wrote:
> > On Thu, Mar 06, 2025 at 12:24:21AM +0000, Benno Lossin wrote:
> >> On Thu Mar 6, 2025 at 12:57 AM CET, Danilo Krummrich wrote:
> >> > On Wed, Mar 05, 2025 at 11:36:54PM +0000, Benno Lossin wrote:
> >> >> On Wed Mar 5, 2025 at 11:38 PM CET, Danilo Krummrich wrote:
> >> >> > On Wed, Mar 05, 2025 at 10:30:31PM +0000, Benno Lossin wrote:
> >> >> >> On Tue Mar 4, 2025 at 6:34 PM CET, Danilo Krummrich wrote:
> >> >> >> > + /// Push an additional path component.
> >> >> >> > + ///
> >> >> >> > + /// After a new [`ModInfoBuilder`] instance has been created, [`ModInfoBuilder::prepare`] must
> >> >> >> > + /// be called before adding path components.
> >> >> >> > + pub const fn push(self, s: &str) -> Self {
> >> >> >> > + if N != 0 && self.n == 0 {
> >> >> >> > + crate::build_error!("Must call prepare() before push().");
> >> >> >>
> >> >> >> This will only prevent the first `prepare` call being missed, right?
> >> >> >
> >> >> > Correct, unfortunately there's no way to detect subsequent ones.
> >> >>
> >> >> Does it make sense to do that one in the constructor?
> >> >>
> >> >> (After looking at the example below) Ah maybe you can't do that, since
> >> >> then you would have two `prepare()` calls for the example below...?
> >> >
> >> > Exactly.
> >> >
> >> >> >> If you always have to call this before `push`, why not inline it there?
> >> >> >
> >> >> > You can push() multiple times to compose the firmware path string (which is the
> >> >> > whole purpose :).
> >> >>
> >> >> Ah I see, I only looked at the example you have in the next patch. All
> >> >> in all, I think this patch could use some better documentation, since I
> >> >> had to read a lot of the code to understand what everything is supposed
> >> >> to do...
> >> >
> >> > I can expand the example in module_firmware! to make things a bit more obvious.
> >> >
> >> > Otherwise, what information do you think is missing?
> >>
> >> Abstractly: what `ModInfoBuilder` *does*, concretely:
> >> - why the generic constant `N` exists,
> >
> > It doesn't really matter to the user, since the user never needs to supply it.
> > That happens in the module_firmware! macro.
> >
> > I agree it not good to not mention anything about it at all, but I wouldn't want
> > to bother the user with all implemention details.
> >
> > We can probably just mention that it's used internally and is supplied by
> > module_firmware!. (That module_firmware! does that by doing a dry run of the
> > builder itself, isn't necessary to know for the user I think.)
> >
> >> - what `prepare()` does,
> >
> > Same here, it's an implementation detail not relevant to the user. All the user
> > needs to know is that prepare() acts as a separator to be able to supply the
> > next firmware path.
>
> How about calling it `new_path`/`new_entry` or similar?
Sure, new_entry() sounds good!
>
> >> - what happens with the `module_name` parameter of `new`
> >
> > Should probably just mention it's supplied by module_firmware! and used
> > internally.
>
> IIUC, that's not the case, the `module_firmware!` macro will call the
> `create` function with the name and you're supposed to just pass it onto
> the builder.
Yes, but this part is documented by module_firmware!, which I think is the
correct place.
>
> >> - answer the question "I want that the builder outputs the string `???`
> >> can it do that? If yes, how do I do it?"
> >
> > All it does is concatenating multiple &str in const context, which I thought is
> > clear since there are only push() and prepare() as public methods.
> >
> > May it be that your request is more about can we add more hints on the
> > implementation details rather than user focused documentation?
>
> I am not familiar with MODULE_FIRMWARE in C, and I'd think that someone
> that uses this API would know what to put into the `.modinfo` section,
> so like "foo\0bar\0\0baz" (no idea if that makes sense, but just add
> `firmware` or whatever is needed to make it make sense). And then the
> question would be how to translate that into the builder.
>
> I wouldn't be able to piece it together without looking at the
> implementation.
I believe if you come from the perspective of writing a driver, you reach
module_firmware! first and then the subsequent stuff makes sense.
But I recognize your feedback and will try to make things a bit more obvious by
expanding the example of module_firmware! and expanding a few comments here and
there. I also think that s/prepare/new_entry/ will help a lot.
