On Thu, May 17, 2018 at 04:09:18PM -0700, Luis R. Rodriguez wrote:
> On Fri, May 18, 2018 at 08:48:49AM +1000, Dave Chinner wrote:
> > On Thu, May 17, 2018 at 12:26:58PM -0700, Luis R. Rodriguez wrote:
> > > Using an enum will let us later just use a switch statement to print
> > > out the source, this makes sources easier to document, update and
> > > manage.
> > >
> > > Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> >
> > This is incomplete. :(
>
> It builds, the comment was intentional.
That doesn't make it complete. Usually when introducing new code one
part at a time, the entire functionality of that part is separated
out so it can be reviewed whole, not split across multilple patches
and intermingled with other new functionality....
> > > /*
> > > * Default filesystem features and configuration values
> > > *
> > > @@ -49,7 +60,7 @@ struct sb_feat_args {
> > > * calculations.
> > > */
> > > struct mkfs_default_params {
> > > - char *source; /* where the defaults came from */
> > > + enum default_params_type type; /* where the defaults came from */
> > >
> > > int sectorsize;
> > > int blocksize;
> >
> > As it is, I don't see why this change it necessary - you can just
> > store the appropriate string (as the code currently does) into the
> > structure once the source is known. Why do we need infrastructure to
> > abstract printing a string when we set it directly, anyway?
>
> Using an enum we get to document the different sources clearly, and we
> also get to have an enum to compare against for conditionals later,
> instead of having to strcmp().
See my comments in later patches, where I suggest all that gets
removed because i don't think it works. :P
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
