lkml.org 
[lkml]   [2009]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 0/10] ide: flags query macros
    Hi,

    > Unfortunately my main concern is still not addressed -- namely the lack of
    > consistency between names of flags and names of inline functions, ie.:
    >
    > - i, (drive->dev_flags & IDE_DFLAG_NO_IO_32BIT) ? "off" : "on");
    > + i, ide_dev_no_32bit_io(drive) ? "off" : "on");
    >
    > This is really the major issue because introduction of this abstraction
    > was supposed to make code more readable and maintainable...
    >
    > With the current version I get exactly the opposite feeling:
    > - we have now different naming used for flags and inline functions
    > - we use inline functions only for checking if flags are set

    Sorry about that, will think of better names and fix.

    > My other complaint is about changing my beloved CodingStyle, i.e.:
    >
    > - if (drive->media != ide_disk ||
    > - (drive->dev_flags & IDE_DFLAG_PRESENT) == 0)
    > + if (drive->media != ide_disk || !ide_dev_present(drive))
    > select |= HT_PREFETCH_MODE;
    >
    > I see '== 0' immediately while it takes a while to notice '!' (funny that
    > long time ago I preferred '!' because it's shorter but in the practice it
    > turns out to be less readable and more prone to cause subtle bugs during
    > code changes, though YMMV).

    :) this is funny, I feel the exact opposite way: If I see the "!" at the
    beginning of the if-clause I just read "not" together with the function
    name so for example for

    if (!ide_dev_present(drive))

    you have "if ide device _not_ present" or even more closely matched word
    order would be "if not ide device present", well, you get the idea.
    That's one of the reasons I was trying to have more readable names
    for those inlines. And this way it is much more natural when reading
    the code instead of "== 0" check where you still have to think a bit :).

    > Also after checking the code I think ide_{d,a}flag_ naming is better
    > as it doesn't overlap with normal ide code...
    >
    > > > diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
    > > > index df3df00..3553759 100644
    > > > --- a/drivers/ide/ide-cd_ioctl.c
    > > > +++ b/drivers/ide/ide-cd_ioctl.c
    > > > @@ -86,7 +86,7 @@ int ide_cdrom_check_media_change_real(struct cdrom_device_info *cdi,
    > > >
    > > > if (slot_nr == CDSL_CURRENT) {
    > > > (void) cdrom_check_status(drive, NULL);
    > > > - retval = (drive->dev_flags & IDE_DFLAG_MEDIA_CHANGED) ? 1 : 0;
    > > > + retval = ide_dev_media_changed(drive) ? 1 : 0;
    > > > drive->dev_flags &= ~IDE_DFLAG_MEDIA_CHANGED;
    > > > return retval;
    > > > } else {
    > > >
    > > > The use of ? 1 : 0; here is redundant.
    > > >
    > > > if (drive->media == ide_disk) {
    > > > - printk(KERN_INFO "%s: non-IDE drive, CHS=%d/%d/%d\n",
    > > > + pr_info("%s: non-IDE drive, CHS=%d/%d/%d\n",
    > > > drive->name, drive->cyl,
    > > > drive->head, drive->sect);
    > > > } else if (drive->media == ide_cdrom) {
    > > > - printk(KERN_INFO "%s: ATAPI cdrom (?)\n", drive->name);
    > > > + pr_info("%s: ATAPI cdrom (?)\n", drive->name);
    > > > } else {
    > > > /* nuke it */
    > > > - printk(KERN_WARNING "%s: Unknown device on bus refused identification. Ignoring.\n",
    > > > +drive->name);
    > > > + pr_warning("%s: Unknown device on bus refused "
    > > > + "identification, ignoring.\n",
    > > > + drive->name);
    > > > I did not see this addressed in the changelog?
    > >
    > > Actually, that was in the v1 changelog but got forgotten. Bart, can you
    > > please add to the changelog
    > >
    > > - shorten >80 lines
    >
    > What about printk() -> pr_info()/pr_warning()?
    >
    > [ Which brings us to consistency issues again -- do you plan to convert
    > whole IDE code to use pr_*()? If yes, great but please do it in separate
    > patches -- I think that converting only some printk()s is not worth it. ]

    Well, I don't know, this could just as well be a kernel janitor task.
    I'll revert to printks here since there's more important stuff to do
    now.

    > Please spend more time on documenting your changes properly.
    >
    > You don't have to write a poem ;) but for reviewer it is important
    > to know if changes are intentional or accidental (since it could be
    > as well unintentional left-over from your private tree)...

    Point taken.

    > > > drive->dev_flags &= ~IDE_DFLAG_PRESENT;
    > > >
    > > > You have a nice set of inlines to facilitate testing bits,
    > > > but not for the above use?
    > > > I guess this was not worth the abstraction for now.
    > >
    > > Yeah, those are next but I'd like to wait a bit until ide rewrite
    > > settles...
    >
    > This should happen in this patch to keep the consistency, moreover since
    > you introduced nice macros to define "test" helpers you can now easily extend
    > them for "clear" ones, i.e.:
    >
    > +#define IDE_AFLAG_(name, flag) \
    > +static inline int ide_test_aflag_##name(ide_drive_t *drive) \
    > +{ \
    > + return !!(drive->atapi_flags & flag); \
    > +} \
    > +static inline void ide_clear_aflag_##name(ide_drive_t *drive) \
    > +{ \
    > + drive->atapi_flags &= ~flag; \
    > +}
    >
    > BTW you may want to delay this patch after 2.6.30 as things should
    > become much more peaceful then.

    What exactly is the timeframe here? Do you want to have the updated
    version for the 2.6.31 merge window?

    --
    Regards/Gruss,
    Boris.

    \
     
     \ /
      Last update: 2009-02-27 07:41    [W:5.175 / U:0.652 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site