Replace rustfmt::skip with alternate construction

[crawford]

This builder construction allows rustfmt to consider each of the individual expressions instead of forcing it to consider them all at once. There are a couple of instances within drv/stm32h7-i2c/src/lib.rs that were not changed since the existing inline-comment style would have been mangled.

I went back and forth on the few instances that I left out. I tried moving the comments to their own line, a style that is used elsewhere in the code base, but it made the block look verbose. I'm not familiar enough with the overall coding style to make a recommendation, but I'd be happy to include additional changes in this pull request if it's desired. Here is the diff for reference:

diff --git a/drv/stm32h7-i2c/src/lib.rs b/drv/stm32h7-i2c/src/lib.rs
index f0055e9..3517d77 100644
--- a/drv/stm32h7-i2c/src/lib.rs
+++ b/drv/stm32h7-i2c/src/lib.rs
@@ -282,17 +282,26 @@ impl<'a> I2cController<'a> {
         self.configure_timing(i2c);
         self.configure_timeouts(i2c);
 
-        #[rustfmt::skip]
-        i2c.cr1.modify(|_, w| { w
-            .smbhen().set_bit()         // enable SMBus host mode
-            .gcen().clear_bit()         // disable General Call
-            .nostretch().clear_bit()    // must enable clock stretching
-            .errie().set_bit()          // emable Error Interrupt
-            .tcie().set_bit()           // enable Transfer Complete interrupt
-            .stopie().clear_bit()       // disable Stop Detection interrupt
-            .nackie().set_bit()         // enable NACK interrupt
-            .rxie().set_bit()           // enable RX interrupt
-            .txie().set_bit()           // enable TX interrupt
+        i2c.cr1.modify(|_, w| {
+            // enable SMBus host mode
+            w.smbhen().set_bit();
+            // disable General Call
+            w.gcen().clear_bit();
+            // must enable clock stretching
+            w.nostretch().clear_bit();
+            // emable Error Interrupt
+            w.errie().set_bit();
+            // enable Transfer Complete interrupt
+            w.tcie().set_bit();
+            // disable Stop Detection interrupt
+            w.stopie().clear_bit();
+            // enable NACK interrupt
+            w.nackie().set_bit();
+            // enable RX interrupt
+            w.rxie().set_bit();
+            // enable TX interrupt
+            w.txie().set_bit();
+            w
         });
 
         i2c.cr1.modify(|_, w| w.pe().set_bit());
@@ -685,29 +694,41 @@ impl<'a> I2cController<'a> {
 
         self.configure_timing(i2c);
 
-        #[rustfmt::skip]
-        i2c.oar1.modify(|_, w| { w
-            .oa1en().clear_bit()                    // own-address disable 
+        i2c.oar1.modify(|_, w| {
+            // own-address disable
+            w.oa1en().clear_bit()
         });
 
-        #[rustfmt::skip]
-        i2c.oar2.modify(|_, w| { w
-            .oa2en().set_bit()                  // own-address-2 enable
-            .oa2msk().bits(0b111)                // mask 7 == match all
+        i2c.oar2.modify(|_, w| {
+            // own-address-2 enable
+            w.oa2en().set_bit();
+            // mask 7 == match all
+            w.oa2msk().bits(0b111);
+            w
         });
 
-        #[rustfmt::skip]
-        i2c.cr1.modify(|_, w| { w
-            .gcen().clear_bit()         // disable General Call
-            .nostretch().clear_bit()    // enable clock stretching
-            .sbc().clear_bit()          // disable byte control 
-            .errie().set_bit()          // emable Error Interrupt
-            .tcie().set_bit()           // enable Transfer Complete interrupt
-            .stopie().set_bit()         // enable Stop Detection interrupt
-            .nackie().set_bit()         // enable NACK interrupt
-            .addrie().set_bit()         // enable Address interrupt
-            .rxie().set_bit()           // enable RX interrupt
-            .txie().set_bit()           // enable TX interrupt
+        i2c.cr1.modify(|_, w| {
+            // disable General Call
+            w.gcen().clear_bit();
+            // enable clock stretching
+            w.nostretch().clear_bit();
+            // disable byte control
+            w.sbc().clear_bit();
+            // emable Error Interrupt
+            w.errie().set_bit();
+            // enable Transfer Complete interrupt
+            w.tcie().set_bit();
+            // enable Stop Detection interrupt
+            w.stopie().set_bit();
+            // enable NACK interrupt
+            w.nackie().set_bit();
+            // enable Address interrupt
+            w.addrie().set_bit();
+            // enable RX interrupt
+            w.rxie().set_bit();
+            // enable TX interrupt
+            w.txie().set_bit();
+            w
         });
 
         i2c.cr1.modify(|_, w| w.pe().set_bit());

[cbiffle]

Interesting. I suppose svd2rust is doing the "kind of a builder" pattern where it's returning &mut Self instead of Self, so this works. (It looks like it forks the builder state and discards the result at each statement.)

This does read better once you know the pattern. I'm open to merging this but I'd like some aesthetic opinions from, say, @labbott and @bcantrill -- what do y'all think?

Still hoping to move away from svd2rust once I get free time. 🤞

[bcantrill]

I'm not really into this change. To me, the existing code is much clearer -- and we have had bugs here in the past because it becomes hard to read. The problem here is rustfmt (and issues like rust-lang/rustfmt#4306) -- and I don't think that the skip here is as much of a problem as the solution. (That is: the cure is worse than the disease.)

[crawford]

Fair enough. Thanks for the consideration.