Re: [PATCH] staging: media: atomisp: Add parentheses around macro definitions

[Dan Carpenter]

On Sat, Aug 03, 2024 at 01:33:43PM +0800, Sean Whitton wrote:
> Hello,
> 
> On Fri 02 Aug 2024 at 11:28pm -05, Dan Carpenter wrote:
> 
> > *You* need to figure out what the proper thing is.  Not us.  That's the
> > difficult part of writing a patch.  Once you know what the correct thing
> > is, then the rest is just typing.
> >
> > That business of defining STORAGE_CLASS_SP_C is weird.  Figure out the
> > authors intention and find a better way to do it.
> >
> > Figure out why your code compiled as well because putting parentheses
> > around (static inline) is a syntax error.
> 
> I asked follow-up questions because it seems like at least partially a
> matter of style to say that the business of defining STORAGE_CLASS_SP_C
> is weird.

I'm a domain expert when it comes to kernel style.  ;)  Trust me, it's
weird.  There are other places which do it as will but it's not normal.

> Maybe there is a better approach than what is currently done,
> but maybe there isn't.

Correct.  Just because it's weird, doesn't mean it's wrong.  Figure out
why the author did what they did and after that you'll probably be able
to judge if it makes sense.

> Maybe the checkpatch warning should just be
> suppressed (if that's something that can be done).

Yes.  Try to suppress the warning.  You don't need anyone's permission.
I think it will be difficult and I doubt you will succeed.  But you
never know until you try.  Even if you don't succeed, it's a useful
exercise.

> I would be grateful for some additional pointers.
> 

Ok.  Here was your question.

> I don't know what the author's intention was.  Are you saying that you
> think this preprocessor mechanism should just be replaced with
> hardcoding 'extern' or 'static inline' in each file which includes one
> of these headers?

The answer is you need to figure out what the author's intention was.
1) Look through the git log.  2) Try removing it and see if anything
breaks.  3) Do a grep for __INLINE_SP__.  (I deleted some extra hints
here because if I give any more hints then it's just me doing the
project).

Once you know why the macro exists then you can decide it we should do a
sed to replace it.  The sed to get rid of the macro is just an automated
one liner thing.  The difficult part is answering why the macro was
created and do we still need it?

regards,
dan carpenter