[libcxx-commits] [PATCH] D139114: [Clang][Sema] Enabled implicit conversion warning for CompoundAssignment operator.

[Nathan Chancellor via Phabricator via libcxx-commits]

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