Re: [PATCH] staging: lustre: lustre_dlm_flags: replace bit constants with BIT_ULL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 21, 2015 at 12:54:54PM +0000, Dilger, Andreas wrote:
> On 2015/09/21, 2:25 PM, "Mike Rapoport" <mike.rapoport@xxxxxxxxx> wrote:
> 
> >Use BIT_ULL() macro instead of long hexadecimal constants with only
> >single bit set.
> >
> >Signed-off-by: Mike Rapoport <mike.rapoport@xxxxxxxxx>
> >---
> > .../lustre/lustre/include/lustre_dlm_flags.h       | 80
> >+++++++++++-----------
> > 1 file changed, 40 insertions(+), 40 deletions(-)
> >
> >diff --git a/drivers/staging/lustre/lustre/include/lustre_dlm_flags.h
> >b/drivers/staging/lustre/lustre/include/lustre_dlm_flags.h
> >index d27bdae0..a247cbf 100644
> >--- a/drivers/staging/lustre/lustre/include/lustre_dlm_flags.h
> >+++ b/drivers/staging/lustre/lustre/include/lustre_dlm_flags.h
> >@@ -50,7 +50,7 @@
> > #define LDLM_FL_ON_WIRE_MASK            0x00000000C08F932FULL
> > 
> > /** extent, mode, or resource changed */
> >-#define LDLM_FL_LOCK_CHANGED            0x0000000000000001ULL // bit   0
> >+#define LDLM_FL_LOCK_CHANGED            BIT_ULL(0)
> 
> IMHO, this is a step backward in usability.  When I see a debug log
> that prints these lock flags in hex, for example 0x200801220, it is
> easier to look at those flags and see it is FL_SKIPPED, FL_TEST_LOCK,
> FL_HAS_INTENT, FL_INTENT_ONLY, FL_AST_SENT.  If they are declared as
> "BIT_ULL(33)" that is not very useful for humans actually looking at
> this code.

I think it's personal and a matter of habbit. In your example,
0x200801220 may be seen as "bits 5, 9, 12, 23 and 33 are set", and then
one may look for bit definitions...

BTW, somewhat semi-related quiestion:
I've grepped for usage of ldlm_{is,set,clear}* and I've found only
three places that are using them. Is there a reason to keep the defines
or replacing those three occurences with open-coded versions and
dropping ldlm_{is,set,clear}* macros would be Ok?
 
> I don't see any benefits to changing these declarations to use BIT_ULL().

The obvoius benifit of these changes is removal of C99 comments :)
 
> Cheers, Andreas
> -- 
> Andreas Dilger
> 
> Lustre Software Architect
> Intel High Performance Data Division

--
Sincerely yours,
Mike.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux