On Mon, 22 Jul 2024 07:58:41 +0200,
Kuninori Morimoto wrote:
>
>
> Hi Amadeusz, Takashi
>
> > > Perhaps you could use generics here, so you could have one caller for
> > > both cases?
> > >
> > > Something like:
> > > #define snd_pcm_is_playback(x) _Generic((x), \
> > > int : (x == SNDRV_PCM_STREAM_PLAYBACK), \
> > > struct snd_pcm_substream *substream * : (x->stream ==
> > > SNDRV_PCM_STREAM_PLAYBACK))(x)
> > > or just call the above functions in it?
>
> Hmm... I couldn't compile above inline style.
> I need to create function, and use it on _Generic().
>
> And then, _Generic() is very picky for variable sytle.
> This means I need to create function for "int" / "const int",
> "unsigned int", "const unsigned int"
>
> static inline int snd_pcm_stream_is_playback_(const int stream)
> {
> return stream == SNDRV_PCM_STREAM_PLAYBACK;
> }
>
> static inline int snd_pcm_stream_is_playback(int stream)
> {
> return stream == SNDRV_PCM_STREAM_PLAYBACK;
> }
>
> static inline int snd_pcm_stream_is_playback_u(const unsigned int stream)
> {
> return stream == SNDRV_PCM_STREAM_PLAYBACK;
> }
>
> static inline int snd_pcm_stream_is_playbacku(unsigned int stream)
> {
> return stream == SNDRV_PCM_STREAM_PLAYBACK;
> }
I really don't see any merit of those inline. If this simplifies the
code, I'd agree, but that's adding just more code...
>
> static inline int snd_pcm_substream_is_playback_(const struct snd_pcm_substream *substream)
> {
> return snd_pcm_stream_is_playback(substream->stream);
> }
>
> static inline int snd_pcm_substream_is_playback(struct snd_pcm_substream *substream)
> {
> return snd_pcm_stream_is_playback(substream->stream);
> }
>
> #define snd_pcm_is_playback(x) _Generic((x), \
> const int : snd_pcm_stream_is_playback_, \
> int : snd_pcm_stream_is_playback, \
> const unsigned int : snd_pcm_stream_is_playback_u, \
> unsigned int : snd_pcm_stream_is_playbacku, \
> const struct snd_pcm_substream * : snd_pcm_substream_is_playback_, \
> struct snd_pcm_substream * : snd_pcm_substream_is_playback)(x)
>
> And I'm not sure how to handle "bit field" by _Generic.
>
> ${LINUX}/sound/pci/ac97/ac97_pcm.c:153:13: note: in expansion of macro 'snd_pcm_is_playback'
> 153 | if (snd_pcm_is_playback(pcm->stream))
> | ^~~~~~~~~~~~~~~~~~~
> ${LINUX}/sound/pci/ac97/ac97_pcm.c: In function 'snd_ac97_pcm_assign':
> ${LINUX}/include/sound/pcm.h:544:41: error: '_Generic' selector of type 'unsigned char:1' is not compatible with any association
> 544 | #define snd_pcm_is_playback(x) _Generic((x), \
>
> Not using _Generic() is easy, "const int" version can handle everything
> (= "int", "const int", "unsigned int", "const unsigned int", "short",
> "const short", "bit field", etc).
>
> If there is better _Generic() grammar, I can follow it.
> If there wasn't, unfortunately let's give up this conversion...
IMO, if the retrieval of the stream direction and its evaluation are
done separately, there is little use of the inline functions like the
above. It doesn't give a better readability nor code safety.
That said, as of now, I'm not much convinced to go further.
OTOH, I'm not strongly against taking this kind of change -- if other
people do find it absolutely better, I'm ready to be convinced :)
thanks,
Takashi
