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

[Dilger, Andreas]

On 2015/09/21, 4:46 PM, "Mike Rapoport" <mike.rapoport at gmail.com> wrote:

>On Mon, Sep 21, 2015 at 12:54:54PM +0000, Dilger, Andreas wrote:
>> On 2015/09/21, 2:25 PM, "Mike Rapoport" <mike.rapoport at gmail.com> wrote:
>> 
>> >Use BIT_ULL() macro instead of long hexadecimal constants with only
>> >single bit set.
>> >
>> >Signed-off-by: Mike Rapoport <mike.rapoport at gmail.com>
>> >---
>> > .../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...

Sure, but I'm the person more likely to be debugging Lustre problems,
so I'd really rather keep it the way it is today.

>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?

They are used a lot in the server code, so they should be kept.

> 
>> 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 :)

No objection to fixing the C99 comments. :-)

Cheers, Andreas
-- 
Andreas Dilger

Lustre Software Architect
Intel High Performance Data Division