Add impl of std::ops::Not to bitmask_binop

[Syudagye]

I noticed bitmasks (e.g. EventMask) was missing the! operator, which can be useful for manipulating this kind of data structure.

[psychon]

Hm, I'm not sure what to make of this. I am usually worried about backwards compatibility (e.g. a new version of an extension introduces a new enum variant and then... that shouldn't change the meaning of existing code), but I can't come up with any case where this would actually break.

What do others think?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b523edd) 12.64% compared to head (6571cec) 12.65%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #899   +/-   ##
=======================================
  Coverage   12.64%   12.65%           
=======================================
  Files         189      189           
  Lines      140456   140468   +12     
=======================================
+ Hits        17767    17781   +14     
+ Misses     122689   122687    -2     

Flag	Coverage Δ
tests	12.65% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[psychon]

I just pushed the result of cargo fmt to make CI happy.

I noticed bitmasks (e.g. EventMask) was missing the! operator, which can be useful for manipulating this kind of data structure.

@Syudagye Could you say something about "can be useful for manipulating this kind of data structure"? What manipulations did you have in mind exactly that isn't possible otherwise?

[notgull]

I'm skeptical of adding more code to be generated, especially since the amount of code we already have is an issue for our downstream consumers. See #883

[Syudagye]

@Syudagye Could you say something about "can be useful for manipulating this kind of data structure"? What manipulations did you have in mind exactly that isn't possible otherwise?

I'm currently trying to make an x11rb backend for LeftWM, and i'm doing it by "transating" the current xlib implementation to x11rb. Doing so requires that i flip masks in some places (like here for example).
For the specific example from above, i need to remove just one flag from a mask constant, and the easiest way to do it is by using NOT (!). Maybe this use case isn't ideal, but right now it is how it is done and i'm just trying to make it work with x11rb.

It may be possible to do this without the NOT operator, but i only see two way:

• redefining the mask constant locally, which duplicates code, and may cause issues if we modify ROOT_EVENT_MASK in the future (which is highly unlikely im).
• using bits(), inverting the value and then using from().

To me this is an expected operation on a mask, like &, |, and even &=, \=

I'm skeptical of adding more code to be generated, especially since the amount of code we already have is an issue for our downstream consumers. See #883

I don't know how much this affects build speeds, and if this is a concern i understand you are chasing every way to not make it worse.
On the other hand, this is (at least to me) a feature i expect from this type of data structure, and it is only adding very few code to be generated, so i don't believe it affect that much the performance.

I could be mistaken though, i have no proper way of testing this other than build times....

[psychon]

Thanks for that input. That seems legitimate to me.

Since I haven't written so before, what is my problem with this? In the XML, enums do not have an inherent size. So, whether an enum is backed by u8 or u16 depends on some heuristics in the code generator and not on anything in the XML. For that reason, the result of a ! is a bit arbitrary meaningless.

Since you only want a & !b, I checked how bitflags does this. Perhaps it would be better to add difference (this is a & !b) and/or a symmetric_difference (a ^ b) method. This would do precisely what you need.

I think I prefer only adding difference instead of both difference and symmetric_difference.

Edit: Random other crate calls this remove. I think I like that name more than difference.

@notgull I know that we are in feature-creep-territory. However, the build time influence seems to be not much (in an unscientific benchmark). cargo clean && time cargo build -p x11rb-protocol --features all-extensions goes for me from 36 to 36 seconds with the following patch (58,71s user 2,26s system 169% cpu 35,994 total -> 58,93s user 2,59s system 170% cpu 36,015 total; only one measurement each):

diff --git a/x11rb-protocol/src/x11_utils.rs b/x11rb-protocol/src/x11_utils.rs
index dcb22c12..c0a86a35 100644
--- a/x11rb-protocol/src/x11_utils.rs
+++ b/x11rb-protocol/src/x11_utils.rs
@@ -684,6 +684,11 @@ macro_rules! bitmask_binop {
             pub fn bits(self) -> $u {
                 self.0
             }
+
+            /// Foobar
+            pub fn difference(self, flag: impl Into<$u>) -> Self {
+                Self::from(<$u>::from(self) & !flag.into())
+            }
         }
     };
 }

[Syudagye]

Well, a difference or remove function would perfectly fit my use-case (and hopefully others' too !).
If that's the better solution then i'm looking forward to see which one you choose to implement.

[notgull]

Okay, I'm fine with adding it for now

[psychon]

I opened #904 for that.

[psychon]

With #904 merged, is that enough for closing this PR? Or is there still something missing?

[psychon]

I'll go with the assumption that #904 superseded this one and everyone is happy with that outcome. Please speak up if my assumption is wrong.