Run cargo fmt #76

stevefan1999-personal · 2023-04-05T11:09:59Z

The change set in #67 looks very long and I don't like that. I realized most of the code are not formatted correctly and as such I blended in cargo fmt result there. Let's make sure the change set is clean first

CensoredUsername · 2023-04-05T14:30:30Z

Hey there,

There's a lot of nice cleanup changes in there, but cargo fmt also messes up the structure / bloats some code significantly. I feel like this needs a manual pass (and/or some use of rustfmt_skip) to not end up making some parts significantly more annoying to read.

stevefan1999-personal · 2023-06-18T07:38:31Z

@CensoredUsername I think we need to find a compromise: we need to rather set an agreement upon the rustfmt configs, so that we can have still have a standard to agree on while being less invasive on code changes.

CensoredUsername · 2023-06-28T19:14:54Z

That makes sense. It's slightly complicated by the fact that I've never used rustfmt on this project, so I do not have a rustfmt config to share here. That said, I really don't have problems with most of the reformatting, but rustfmt's insistence to unroll long enum declarations unnecessary, and its prospensity to remove vertically aligned spacing where it really adds readability is a problem. If just those sections were rustfmt_skip exempted I'd be fine with this changeset. I'll try provide some pointers to what I mean below.

CensoredUsername

I hope this helps clarify the few points that I have problems with.

I didn't label every change because for instance, there's a lot of register listings that just need a rustfmt_skip. But it should give a decent idea of what I mean.

CensoredUsername · 2023-06-28T19:17:12Z

plugin/src/arch/aarch64/aarch64data.rs

@@ -83,11 +83,11 @@ pub enum Matcher {
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum Command {
    // commands that advance the argument pointer
-    R(u8), // encode a register, or reference base, into a 5-bit bitfield.
+    R(u8),     // encode a register, or reference base, into a 5-bit bitfield.


This is what I mean with rustfmt disliking vertical alignment. The comments here were indented to the same line for readability.

CensoredUsername · 2023-06-28T19:19:33Z

plugin/src/arch/aarch64/ast.rs

-    X20= 0x14, X21= 0x15, X22= 0x16, X23= 0x17,
-    X24= 0x18, X25= 0x19, X26= 0x1A, X27= 0x1B,
-    X28= 0x1C, X29= 0x1D, X30= 0x1E,
+    X0 = 0x00,


And here is an example of it unrolling large enums for little reason. This doesn't make the file easier to read. dynasm-rs contains a significant amount of these tables, and the file really doesn't get more readable from multiple chunks of 32 lines of just register numbering.

CensoredUsername · 2023-06-28T19:20:06Z

plugin/src/arch/aarch64/ast.rs

    INTEGERSP = 1,
-    SIMD      = 2,
+    SIMD = 2,


Another example of rustfmt ignoring vertical alignment.

CensoredUsername · 2023-06-28T19:26:27Z

plugin/src/arch/aarch64/debug.rs

-                Matcher::VElement(s) => write!(buf, "V{}.{}[{}]", arg_names[0], size_to_string(*s), arg_names[1]).unwrap(),
-                Matcher::VElementStatic(s, element) => write!(buf, "V{}.{}[{}]", arg_names[0], size_to_string(*s), element).unwrap(),
-                Matcher::VStaticElement(s, c) => write!(buf, "V{}.{}{}[{}]", arg_names[0], size_to_string(*s), c, arg_names[1]).unwrap(),
+                    write!(


These ones I'm a bit on edge about. I think they're being triggered by a column width check? Looks like it rejects over 100 characters. Personally I'm fine with 120 at least, although the last does go over that so maybe just exempting this section is for the best.

CensoredUsername · 2023-06-28T19:28:09Z

plugin/src/arch/aarch64/parser.rs

-            ("x7" , (X7 , Some(QWORD))),
-            ("x8" , (X8 , Some(QWORD))),
-            ("x9" , (X9 , Some(QWORD))),
+            ("x0", (X0, Some(QWORD))),


Vertical alignment shenanigans again.

CensoredUsername · 2023-06-28T19:39:23Z

plugin/src/serialize.rs

+                    kind,
+                ],
+            ),
+            Stmt::PrefixStmt(s) | Stmt::Stmt(s) => {


I'm torn on this one, the original had quite long lines but this feels a bit overkill as well.

CensoredUsername · 2023-06-28T19:39:40Z

plugin/src/serialize.rs

+                    ref_offset,
+                    ..
+                },
+            ) => {


This is just bloat.

CensoredUsername · 2023-06-28T19:40:03Z

runtime/src/aarch64.rs

+    X28 = 0x1C,
+    X29 = 0x1D,
+    X30 = 0x1E,
+    XZR = 0x1F,


And we've seen this before.

CensoredUsername · 2023-06-28T19:41:19Z

runtime/src/components.rs

+                20, 0, 0x00, 0x00, 16, 0, 0, 0, 0x00, 0x00, 0x00, 0x00, 0xD8, 0xFF, 0xFF, 0xFF,
+                0xFF, 0xFF, 0xFF, 0xFFu8,
+            ] as &[u8]
+        );


The grouping by 8's was completely intended here.

CensoredUsername · 2023-06-28T19:42:41Z

runtime/src/x64.rs

+    CR12 = 0xC,
+    CR13 = 0xD,
+    CR14 = 0xE,
+    CR15 = 0xF,


And yeah, these are both in the runtime and plugin crates. Can't import from a plugin and a dependency the other way...

vext01 · 2024-03-25T15:02:05Z

FWIW, I accidentally ran cargo fmt when doing #92, not realising that we don't use auto-formatting in this repo. I had to then pick my changes apart from the unrelated formatting changes.

Personally I like rustfmt. I think there is worth in everyone being on the same page WRT formatting, but I also respect upstream's decision to choose whether to use it or not.

If we are not using it, I wonder if there's a way to prevent cargo fmt from doing anything so that others don't make the same mistake as I did?

CensoredUsername · 2024-03-25T18:40:47Z

@vext01 I've just not used it in this repo before, as I didn't feel the need for it when it was mostly me using it. If people want to use it I'm fine with it (see this pull), but there's some places where cargo fmt makes the code significantly harder to read, and I'd like those to be addressed instead of people just blanket running cargo fmt, not looking at how it affects the output, and adding exemptions for where it is needed.

I wrote down some notes on where it felt necessary to me in the requested changes above if you're interested.

run cargo fmt

4082a38

CensoredUsername requested changes Jun 28, 2023

View reviewed changes

CensoredUsername mentioned this pull request Nov 11, 2024

Run rustfmt and clippy? #104

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run cargo fmt #76

Run cargo fmt #76

stevefan1999-personal commented Apr 5, 2023

CensoredUsername commented Apr 5, 2023

stevefan1999-personal commented Jun 18, 2023

CensoredUsername commented Jun 28, 2023

CensoredUsername left a comment •

edited

Loading

CensoredUsername Jun 28, 2023

CensoredUsername Jun 28, 2023

CensoredUsername Jun 28, 2023

CensoredUsername Jun 28, 2023

CensoredUsername Jun 28, 2023

CensoredUsername Jun 28, 2023

CensoredUsername Jun 28, 2023

CensoredUsername Jun 28, 2023

CensoredUsername Jun 28, 2023

CensoredUsername Jun 28, 2023

vext01 commented Mar 25, 2024

CensoredUsername commented Mar 25, 2024

Run cargo fmt #76

Are you sure you want to change the base?

Run cargo fmt #76

Conversation

stevefan1999-personal commented Apr 5, 2023

CensoredUsername commented Apr 5, 2023

stevefan1999-personal commented Jun 18, 2023

CensoredUsername commented Jun 28, 2023

CensoredUsername left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vext01 commented Mar 25, 2024

CensoredUsername commented Mar 25, 2024

CensoredUsername left a comment •

edited

Loading