On 2017-11-29, at 03:04:53 +0300, Dan Carpenter wrote:
> On Tue, Nov 28, 2017 at 11:33:37PM +0000, Jeremy Sowden wrote:
> > On 2017-11-28, at 17:15:24 +0300, Dan Carpenter wrote:
> > > On Mon, Nov 27, 2017 at 12:44:48PM +0000, Jeremy Sowden wrote:
> > > > The "address" member of struct ia_css_host_data is a
> > > > pointer-to-char, so define default as NULL.
> > > >
> > > > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h
> > > > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h
> > > > @@ -95,7 +95,7 @@ union ia_css_all_memory_offsets {
> > > > };
> > > >
> > > > #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \
> > > > - { { { { 0, 0 } } } }
> > > > + { { { { NULL, 0 } } } }
> > >
> > > This define is way ugly and instead of making superficial changes, you
> > > should try to eliminate it.
> > >
> > > People look at warnings as a bad thing but they are actually a
> > > valuable resource which call attention to bad code. By making this
> > > change you're kind of wasting the warning. The bad code is still
> > > there, it's just swept under the rug but like a dead mouse carcass
> > > it's still stinking up the living room. We should leave the warning
> > > there until it irritates someone enough to fix it properly.
> >
> > Tracking down the offending initializer was definitely a pain.
> >
> > Compound literals with designated initializers would make this macro
> > (and a number of others) easier to understand and more type-safe:
> >
> > #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \
> > - { { { { 0, 0 } } } }
> > + (struct ia_css_isp_param_host_segments) { \
> > + .params = { { \
> > + (struct ia_css_host_data) { \
> > + .address = NULL, \
> > + .size = 0 \
> > + } \
> > + } } \
> > + }
>
> Using designated initializers is good, yes. Can't we just use an
> empty initializer since this is all zeroed memory anyway?
>
> (struct ia_css_isp_param_host_segments) { }
>
> I haven't tried it.
There are 35 defaults defined by macros like this, most of them much
more complicated that IA_CSS_DEFAULT_ISP_MEM_PARAMS, and a few members
are initialized to non-zero values. My plan, therefore, is to convert
everything to use designated initializers, and then start removing the
zeroes afterwards.
> >
> > Unfortunately this default value is one end of a chain of default values
>
> Yeah. A really long chain...
>
> > used to initialize members of default values of enclosing structs where
> > the outermost values are used to initialize some static variables:
> >
> > static enum ia_css_err
> > init_pipe_defaults(enum ia_css_pipe_mode mode,
> > struct ia_css_pipe *pipe,
> > bool copy_pipe)
> > {
> > static struct ia_css_pipe default_pipe = IA_CSS_DEFAULT_PIPE;
> > static struct ia_css_preview_settings prev = IA_CSS_DEFAULT_PREVIEW_SETTINGS;
> > static struct ia_css_capture_settings capt = IA_CSS_DEFAULT_CAPTURE_SETTINGS;
> > static struct ia_css_video_settings video = IA_CSS_DEFAULT_VIDEO_SETTINGS;
> > static struct ia_css_yuvpp_settings yuvpp = IA_CSS_DEFAULT_YUVPP_SETTINGS;
> >
> > if (pipe == NULL) {
> > IA_CSS_ERROR("NULL pipe parameter");
> > return IA_CSS_ERR_INVALID_ARGUMENTS;
> > }
> >
> > /* Initialize pipe to pre-defined defaults */
> > *pipe = default_pipe;
> >
> > [...]
> >
> > I'm not convinced, however, that those variables actually achieve very
> > much. If I change the code to assign the defaults directly, the problem
> > goes away:
> >
> > [...]
> >
> > Does this seem reasonable or am I barking up the wrong tree?
>
> Yes. Chopping the chain down and deleting as much of this code as
> possible seems a good thing.
I'll get chopping.
J.
Attachment:
signature.asc
Description: PGP signature
