Page MenuHomeFreeBSD

sound: Clean up pcm/ includes
AbandonedPublic

Authored by christos on Dec 2 2024, 5:18 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 4, 6:18 AM
Unknown Object (File)
Thu, Dec 26, 7:42 AM
Unknown Object (File)
Thu, Dec 26, 7:38 AM
Unknown Object (File)
Thu, Dec 26, 2:58 AM
Unknown Object (File)
Fri, Dec 13, 7:22 AM
Unknown Object (File)
Dec 6 2024, 11:31 AM
Unknown Object (File)
Dec 4 2024, 9:51 PM
Unknown Object (File)
Dec 4 2024, 9:51 PM
Subscribers

Details

Summary
  • Group pcm/ header files in pcm/sound.h instead of repeating in multiple files.
  • Re-organize sections in pcm/sound.h and sort alphabetically.
  • Get rid of unnecessary includes.
  • Include opt_snd.h once in pcm/sound.h, instead of every single file.
  • Move all includes at the top of each file.

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60918
Build 57802: arc lint + arc unit

Event Timeline

Not really sure what to do with SND_USE_FXDIV and SND_DECLARE_FXDIV. Also I will write some follow-up patches to clean up the drivers as well.

I have to admit the centralization through sound.h goes against my intuition - usually I would only include what is needed, where it's actually used. While this change reduces the number of includes, it makes it even harder to see the dependencies and layers in the sound module. But that's just my opinion, I suppose @markj and @emaste may be the better judges in this case.

I have to admit the centralization through sound.h goes against my intuition - usually I would only include what is needed, where it's actually used. While this change reduces the number of includes, it makes it even harder to see the dependencies and layers in the sound module. But that's just my opinion, I suppose @markj and @emaste may be the better judges in this case.

While I kind of agree, I notice that some of the includes are simply repeated over and over, so IMHO it's better to just have them in one place. After all, we include pcm/sound.h everywhere anyway. Regarding dependencies and layering, I intentionally excluded pcm/sndstat.c-specific includes since they are not needed anywhere else, as well the foo_if.h ones, which I kept/moved to their respective header files.

An alternative proposal is that we strip pcm/sound.h of any unnecessary includes, and instead do what you said: include only what is needed, where it's needed.

I have to admit the centralization through sound.h goes against my intuition - usually I would only include what is needed, where it's actually used. While this change reduces the number of includes, it makes it even harder to see the dependencies and layers in the sound module. But that's just my opinion, I suppose @markj and @emaste may be the better judges in this case.

While I kind of agree, I notice that some of the includes are simply repeated over and over, so IMHO it's better to just have them in one place. After all, we include pcm/sound.h everywhere anyway. Regarding dependencies and layering, I intentionally excluded pcm/sndstat.c-specific includes since they are not needed anywhere else, as well the foo_if.h ones, which I kept/moved to their respective header files.

Regarding foo_if.h, since the foo.h include them now they are also transitively included in sound.h, and thus everywhere. I agree we already include a lot of things in most places, so it's probably not a big issue. But I'd like to know about FreeBSD kernel best practice.

@markj What do you think?

I like the removal of unnecessary includes in .c files. That can be done as its own commit, I think.

Headers themselves should aim to be self-contained, i.e., they shouldn't require additional headers to be included first. We used to not do that, but we're gradually changing to make headers self-contained.

I am skeptical that the rest of the diff has much value though. I'm not too familiar with the interfaces in question, but in general having separate headers makes it easier to catch layering violations and think about interface design. I also can't immediately see whether userspace applications might be making use of these headers, in which case we should avoid breaking them.

sys/dev/sound/pcm/pcm.h
32

Why remove this? It's needed for the typedefs below. (Actually, sys/types.h is sufficient I think.) In general we try to make headers self-contained.