search

davidker / unisys

Master repository for new changes to drivers/staging/unisys and drivers/visorbus
Other
2 stars 1 forks source link

fix remaining few checkpatch errors in visorhba_main.c #52

Closed selltc closed 8 years ago

selltc commented 8 years ago

KanBoard-1117

ERROR: Macros with complex values should be enclosed in parentheses
#143: FILE: visorhba/visorhba_main.c:143:
+#define for_each_vdisk_match(iter, list, match)                          \
+       for (iter = &list->head; iter->next; iter = iter->next) \
+               if ((iter->channel == match->channel) &&                  \
+                   (iter->id == match->id) &&                    \
+                   (iter->lun == match->lun))

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#1070: FILE: visorhba/visorhba_main.c:1070:
+       scsihost->max_id = (unsigned)max.max_id;

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#1071: FILE: visorhba/visorhba_main.c:1071:
+       scsihost->max_lun = (unsigned)max.max_lun;

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#1072: FILE: visorhba/visorhba_main.c:1072:
+       scsihost->cmd_per_lun = (unsigned)max.cmd_per_lun;

total: 1 errors, 3 warnings, 1211 lines checked

visorhba/visorhba_main.c has style problems, please review.
davidker commented 8 years ago

Warning: The error message might be more complex than it looks. We do have an exemption from the upstream for that one if need be. It could be a checkpatch problem or it needs to be fully refactored in the code. The other ones should be easier.

selltc commented 8 years ago

The ERROR: Macros with complex values should be enclosed in parentheses is NOT readily fixable. From the checkpatch code, it is trying to say that we should be using "do {} while(0)" for a multi-line macro, but that does NOT apply to this scenario. This is NOT your garden-variety multi-line macro.

davidker commented 8 years ago

Correct, I am fine with leaving that ERROR in the mix. It will definitely not be easy.

selltc commented 8 years ago

If we HAD to fix it, I would separate into 2 macros, to be used like this:

for_each_vdisk(vdisk, devdata)
    if (vdisk_match(vdisk, scsidev)) {
...

But that would tend to unnecessarily bloat+indent about 5 different places in the code.

davidker commented 8 years ago

Exactly, so I'm not worried about that one.

selltc commented 8 years ago

Dag... I though maybe I could trick checkpatch by simplifying the code like this:

#define for_each_vdisk(iter, list) \
       for (iter = &list->head; iter->next; iter = iter->next)
#define for_each_vdisk_match(iter, list, match) \
       for_each_vdisk(iter, list)              \
               if (vdisk_match(iter, match))

static bool vdisk_match(struct visordisk_info *vdisk,
                       struct scsi_device *scsidev)
{
       return (vdisk->channel == scsidev->channel) &&
               (vdisk->id == scsidev->id) &&
               (vdisk->lun == scsidev->lun);
}

...but checkpatch STILL complains.

Oh, well... I'll give up on this for now.

selltc commented 8 years ago

commit 5cc3988f30363e5e24830aad3c3e35a2185203e9 is checkpatch-clean and compile-clean.

selltc commented 8 years ago

Greg added to staging-testing, then to staging-next a few hours later: