nathanchance added a comment.

> In D139114#4053166 <https://reviews.llvm.org/D139114#4053166>, @nathanchance 
> wrote:
>
>> For what it’s worth, this triggers a LOT in the Linux kernel for the pattern 
>> that @MaskRay pointed out:
>
> Out of curiosity, has it found any true positives that you can tell?

I have not been able to look too closely since I am on mobile until Wednesday 
but of the instances I examined, they all appear to be false positives (or 
maybe true positives that we really just don’t care about) .

The Linux kernel has a macro `BIT(x)`, which is just `1UL << x`, so any 
negation is going to be a really large number but expected to truncate when 
assigning to a narrower width variable because the bit being cleared might only 
be in the first few positions. That is at least the case with the really noisy 
instance of the warning that I mentioned above

  #define FWNODE_FLAG_LINKS_ADDED                       BIT(0)
  #define FWNODE_FLAG_NOT_DEVICE                        BIT(1)
  #define FWNODE_FLAG_INITIALIZED                       BIT(2)
  #define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD  BIT(3)
  #define FWNODE_FLAG_BEST_EFFORT                       BIT(4)
  
  struct fwnode_handle {
        struct fwnode_handle *secondary;
        const struct fwnode_operations *ops;
        struct device *dev;
        struct list_head suppliers;
        struct list_head consumers;
        u8 flags;
  };
  …
  if (initialized)
        fwnode->flags |= FWNODE_FLAG_INITIALIZED;
  else
        fwnode->flags &= ~FWNODE_FLAG_INITIALIZED;



>> It would be nice if this was split into a separate flag that could be 
>> disabled, as -Wconstant-conversion has not been very noisy up until this 
>> point.
>
> We can definitely split it into a separate flag. Alternatively, we could 
> investigate disabling the warning for that code pattern (or moving that bit 
> under its own flag).

Right, I would suspect that emitting this for `&=` will rarely show anything 
interesting. I’ll have to filter the warnings to see if there are any other 
instances with other operators that appear problematic. I’ll see how easy that 
is or if there is a simple way to omit those instances that can be added to the 
patch itself for testing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139114/new/

https://reviews.llvm.org/D139114

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to