thejoshwolfe
commented
1 year ago To give some visuals to this proposal, here's what would happen to this block of code: https://github.com/ziglang/zig/blob/8fb4a4efbabbe3fe98521201eac41feee5a9a50a/lib/std/multi_array_list.zig#L687-L753
try list.ensureTotalCapacity(ally, 20);
try list.append(ally, .{ .tag = .keyword_const , .start = 0 });
try list.append(ally, .{ .tag = .identifier , .start = 6 });
try list.append(ally, .{ .tag = .equal , .start = 10 });
try list.append(ally, .{ .tag = .builtin , .start = 12 });
try list.append(ally, .{ .tag = .l_paren , .start = 19 });
try list.append(ally, .{ .tag = .string_literal, .start = 20 });
try list.append(ally, .{ .tag = .r_paren , .start = 25 });
try list.append(ally, .{ .tag = .semicolon , .start = 26 });
try list.append(ally, .{ .tag = .keyword_pub , .start = 29 });
try list.append(ally, .{ .tag = .keyword_fn , .start = 33 });
try list.append(ally, .{ .tag = .identifier , .start = 36 });
try list.append(ally, .{ .tag = .l_paren , .start = 40 });
try list.append(ally, .{ .tag = .r_paren , .start = 41 });
try list.append(ally, .{ .tag = .identifier , .start = 43 });
try list.append(ally, .{ .tag = .bang , .start = 51 });
try list.append(ally, .{ .tag = .identifier , .start = 52 });
try list.append(ally, .{ .tag = .l_brace , .start = 57 });
try list.append(ally, .{ .tag = .identifier , .start = 63 });
try list.append(ally, .{ .tag = .period , .start = 66 });
try list.append(ally, .{ .tag = .identifier , .start = 67 });
try list.append(ally, .{ .tag = .period , .start = 70 });
try list.append(ally, .{ .tag = .identifier , .start = 71 });
try list.append(ally, .{ .tag = .l_paren , .start = 75 });
try list.append(ally, .{ .tag = .string_literal, .start = 76 });
try list.append(ally, .{ .tag = .comma , .start = 113 });
try list.append(ally, .{ .tag = .period , .start = 115 });
try list.append(ally, .{ .tag = .l_brace , .start = 116 });
try list.append(ally, .{ .tag = .r_brace , .start = 117 });
try list.append(ally, .{ .tag = .r_paren , .start = 118 });
try list.append(ally, .{ .tag = .semicolon , .start = 119 });
try list.append(ally, .{ .tag = .r_brace , .start = 121 });
try list.append(ally, .{ .tag = .eof , .start = 123 });Note that if there was no blank line after the first line, then the alignment wouldn't kick in. Consequentially, nothing would happen to the block of testing.expect calls, because there is no blank line separating the tags assignment with the rest of the lines.
Another example from the same file: https://github.com/ziglang/zig/blob/8fb4a4efbabbe3fe98521201eac41feee5a9a50a/lib/std/multi_array_list.zig#L870-L877
The proposal here would not solve this alignment use case, because 'b' and 1 have different token tags (i believe i know what a token tag is.). So for this block of code, we'd still need // zig fmt: off.
One more exercise: I was curious what would happen if we removed the first clause of the specification about separating into intentional blocks, and I tried formatting the entire multi_array_list.zig file using only the second rule: Any sequence of consecutive lines that have the same sequence of token tags gets aligned. To be clear, this is not what is being proposed here, but it gives illustrations of how things could be aligned if blank lines were inserted in various places, which I think is interesting.
diff --git a/lib/std/multi_array_list.zig b/lib/std/multi_array_list.zig
index 8b5df4a2e..0cb694240 100644
--- a/lib/std/multi_array_list.zig
+++ b/lib/std/multi_array_list.zig
@@ -1,10 +1,10 @@
-const std = @import("std");
+const std = @import("std" );
const builtin = @import("builtin");
const assert = std.debug.assert;
-const meta = std.meta;
-const mem = std.mem;
+const meta = std.meta ;
+const mem = std.mem ;
const Allocator = mem.Allocator;
-const testing = std.testing;
+const testing = std.testing ;
/// A MultiArrayList stores a list of a struct or tagged union type.
/// Instead of storing a single list of items, MultiArrayList
@@ -20,7 +20,7 @@ const testing = std.testing;
pub fn MultiArrayList(comptime T: type) type {
return struct {
bytes: [*]align(@alignOf(T)) u8 = undefined,
- len: usize = 0,
+ len : usize = 0,
capacity: usize = 0,
const Elem = switch (@typeInfo(T)) {
@@ -35,7 +35,7 @@ pub fn MultiArrayList(comptime T: type) type {
} });
pub const Tag =
u.tag_type orelse @compileError("MultiArrayList does not support untagged unions");
- tags: Tag,
+ tags: Tag ,
data: Bare,
pub fn fromT(outer: T) @This() {
@@ -66,7 +66,7 @@ pub fn MultiArrayList(comptime T: type) type {
/// This array is indexed by the field index which can be obtained
/// by using @intFromEnum() on the Field enum
ptrs: [fields.len][*]u8,
- len: usize,
+ len : usize,
capacity: usize,
pub fn items(self: Slice, comptime field: Field) []FieldType(field) {
@@ -113,7 +113,7 @@ pub fn MultiArrayList(comptime T: type) type {
const aligned_ptr: [*]align(@alignOf(Elem)) u8 = @alignCast(unaligned_ptr);
return .{
.bytes = aligned_ptr,
- .len = self.len,
+ .len = self.len,
.capacity = self.capacity,
};
}
@@ -141,9 +141,9 @@ pub fn MultiArrayList(comptime T: type) type {
/// `sizes.fields` is an array mapping from `sizes.bytes` array index to field index.
const sizes = blk: {
const Data = struct {
- size: usize,
+ size : usize,
size_index: usize,
- alignment: usize,
+ alignment : usize,
};
var data: [fields.len]Data = undefined;
for (fields, 0..) |field_info, i| {
@@ -160,14 +160,14 @@ pub fn MultiArrayList(comptime T: type) type {
}
};
mem.sort(Data, &data, {}, Sort.lessThan);
- var sizes_bytes: [fields.len]usize = undefined;
+ var sizes_bytes : [fields.len]usize = undefined;
var field_indexes: [fields.len]usize = undefined;
for (data, 0..) |elem, i| {
- sizes_bytes[i] = elem.size;
+ sizes_bytes [i] = elem.size ;
field_indexes[i] = elem.size_index;
}
break :blk .{
- .bytes = sizes_bytes,
+ .bytes = sizes_bytes ,
.fields = field_indexes,
};
};
@@ -347,7 +347,7 @@ pub fn MultiArrayList(comptime T: type) type {
return;
}
assert(new_len <= self.capacity);
- assert(new_len <= self.len);
+ assert(new_len <= self.len );
const other_bytes = gpa.alignedAlloc(
u8,
@@ -371,10 +371,10 @@ pub fn MultiArrayList(comptime T: type) type {
var other = Self{
.bytes = other_bytes.ptr,
.capacity = new_len,
- .len = new_len,
+ .len = new_len,
};
self.len = new_len;
- const self_slice = self.slice();
+ const self_slice = self .slice();
const other_slice = other.slice();
inline for (fields, 0..) |field_info, i| {
if (@sizeOf(field_info.type) != 0) {
@@ -555,7 +555,7 @@ pub fn MultiArrayList(comptime T: type) type {
.type = *fields[i].type,
.default_value = null,
.is_comptime = fields[i].is_comptime,
- .alignment = fields[i].alignment,
+ .alignment = fields[i].alignment ,
};
break :entry @Type(.{ .Struct = .{
.layout = .Extern,
@@ -567,7 +567,7 @@ pub fn MultiArrayList(comptime T: type) type {
/// This function is used in the debugger pretty formatters in tools/ to fetch the
/// child field order and entry type to facilitate fancy debug printing for this type.
fn dbHelper(self: *Self, child: *Elem, field: *Field, entry: *Entry) void {
- _ = self;
+ _ = self ;
_ = child;
_ = field;
_ = entry;
@@ -627,8 +627,8 @@ test "basic usage" {
try testing.expectEqualSlices(u8, list.items(.c), &[_]u8{ 'a', 'b', 'c' });
try testing.expectEqual(@as(usize, 3), list.items(.b).len);
- try testing.expectEqualStrings("foobar", list.items(.b)[0]);
- try testing.expectEqualStrings("zigzag", list.items(.b)[1]);
+ try testing.expectEqualStrings("foobar", list.items(.b)[0]);
+ try testing.expectEqualStrings("zigzag", list.items(.b)[1]);
try testing.expectEqualStrings("fizzbuzz", list.items(.b)[2]);
// Add 6 more things to force a capacity increase.
@@ -658,8 +658,8 @@ test "basic usage" {
try testing.expectEqualSlices(u8, list.items(.c), &[_]u8{ 'a', 'b', 'c' });
try testing.expectEqual(@as(usize, 3), list.items(.b).len);
- try testing.expectEqualStrings("foobar", list.items(.b)[0]);
- try testing.expectEqualStrings("zigzag", list.items(.b)[1]);
+ try testing.expectEqualStrings("foobar", list.items(.b)[0]);
+ try testing.expectEqualStrings("zigzag", list.items(.b)[1]);
try testing.expectEqualStrings("fizzbuzz", list.items(.b)[2]);
list.set(try list.addOne(ally), .{
@@ -686,71 +686,71 @@ test "regression test for @reduce bug" {
try list.ensureTotalCapacity(ally, 20);
- try list.append(ally, .{ .tag = .keyword_const, .start = 0 });
- try list.append(ally, .{ .tag = .identifier, .start = 6 });
- try list.append(ally, .{ .tag = .equal, .start = 10 });
- try list.append(ally, .{ .tag = .builtin, .start = 12 });
- try list.append(ally, .{ .tag = .l_paren, .start = 19 });
- try list.append(ally, .{ .tag = .string_literal, .start = 20 });
- try list.append(ally, .{ .tag = .r_paren, .start = 25 });
- try list.append(ally, .{ .tag = .semicolon, .start = 26 });
- try list.append(ally, .{ .tag = .keyword_pub, .start = 29 });
- try list.append(ally, .{ .tag = .keyword_fn, .start = 33 });
- try list.append(ally, .{ .tag = .identifier, .start = 36 });
- try list.append(ally, .{ .tag = .l_paren, .start = 40 });
- try list.append(ally, .{ .tag = .r_paren, .start = 41 });
- try list.append(ally, .{ .tag = .identifier, .start = 43 });
- try list.append(ally, .{ .tag = .bang, .start = 51 });
- try list.append(ally, .{ .tag = .identifier, .start = 52 });
- try list.append(ally, .{ .tag = .l_brace, .start = 57 });
- try list.append(ally, .{ .tag = .identifier, .start = 63 });
- try list.append(ally, .{ .tag = .period, .start = 66 });
- try list.append(ally, .{ .tag = .identifier, .start = 67 });
- try list.append(ally, .{ .tag = .period, .start = 70 });
- try list.append(ally, .{ .tag = .identifier, .start = 71 });
- try list.append(ally, .{ .tag = .l_paren, .start = 75 });
- try list.append(ally, .{ .tag = .string_literal, .start = 76 });
- try list.append(ally, .{ .tag = .comma, .start = 113 });
- try list.append(ally, .{ .tag = .period, .start = 115 });
- try list.append(ally, .{ .tag = .l_brace, .start = 116 });
- try list.append(ally, .{ .tag = .r_brace, .start = 117 });
- try list.append(ally, .{ .tag = .r_paren, .start = 118 });
- try list.append(ally, .{ .tag = .semicolon, .start = 119 });
- try list.append(ally, .{ .tag = .r_brace, .start = 121 });
- try list.append(ally, .{ .tag = .eof, .start = 123 });
+ try list.append(ally, .{ .tag = .keyword_const , .start = 0 });
+ try list.append(ally, .{ .tag = .identifier , .start = 6 });
+ try list.append(ally, .{ .tag = .equal , .start = 10 });
+ try list.append(ally, .{ .tag = .builtin , .start = 12 });
+ try list.append(ally, .{ .tag = .l_paren , .start = 19 });
+ try list.append(ally, .{ .tag = .string_literal, .start = 20 });
+ try list.append(ally, .{ .tag = .r_paren , .start = 25 });
+ try list.append(ally, .{ .tag = .semicolon , .start = 26 });
+ try list.append(ally, .{ .tag = .keyword_pub , .start = 29 });
+ try list.append(ally, .{ .tag = .keyword_fn , .start = 33 });
+ try list.append(ally, .{ .tag = .identifier , .start = 36 });
+ try list.append(ally, .{ .tag = .l_paren , .start = 40 });
+ try list.append(ally, .{ .tag = .r_paren , .start = 41 });
+ try list.append(ally, .{ .tag = .identifier , .start = 43 });
+ try list.append(ally, .{ .tag = .bang , .start = 51 });
+ try list.append(ally, .{ .tag = .identifier , .start = 52 });
+ try list.append(ally, .{ .tag = .l_brace , .start = 57 });
+ try list.append(ally, .{ .tag = .identifier , .start = 63 });
+ try list.append(ally, .{ .tag = .period , .start = 66 });
+ try list.append(ally, .{ .tag = .identifier , .start = 67 });
+ try list.append(ally, .{ .tag = .period , .start = 70 });
+ try list.append(ally, .{ .tag = .identifier , .start = 71 });
+ try list.append(ally, .{ .tag = .l_paren , .start = 75 });
+ try list.append(ally, .{ .tag = .string_literal, .start = 76 });
+ try list.append(ally, .{ .tag = .comma , .start = 113 });
+ try list.append(ally, .{ .tag = .period , .start = 115 });
+ try list.append(ally, .{ .tag = .l_brace , .start = 116 });
+ try list.append(ally, .{ .tag = .r_brace , .start = 117 });
+ try list.append(ally, .{ .tag = .r_paren , .start = 118 });
+ try list.append(ally, .{ .tag = .semicolon , .start = 119 });
+ try list.append(ally, .{ .tag = .r_brace , .start = 121 });
+ try list.append(ally, .{ .tag = .eof , .start = 123 });
const tags = list.items(.tag);
- try testing.expectEqual(tags[1], .identifier);
- try testing.expectEqual(tags[2], .equal);
- try testing.expectEqual(tags[3], .builtin);
- try testing.expectEqual(tags[4], .l_paren);
- try testing.expectEqual(tags[5], .string_literal);
- try testing.expectEqual(tags[6], .r_paren);
- try testing.expectEqual(tags[7], .semicolon);
- try testing.expectEqual(tags[8], .keyword_pub);
- try testing.expectEqual(tags[9], .keyword_fn);
- try testing.expectEqual(tags[10], .identifier);
- try testing.expectEqual(tags[11], .l_paren);
- try testing.expectEqual(tags[12], .r_paren);
- try testing.expectEqual(tags[13], .identifier);
- try testing.expectEqual(tags[14], .bang);
- try testing.expectEqual(tags[15], .identifier);
- try testing.expectEqual(tags[16], .l_brace);
- try testing.expectEqual(tags[17], .identifier);
- try testing.expectEqual(tags[18], .period);
- try testing.expectEqual(tags[19], .identifier);
- try testing.expectEqual(tags[20], .period);
- try testing.expectEqual(tags[21], .identifier);
- try testing.expectEqual(tags[22], .l_paren);
+ try testing.expectEqual(tags[ 1], .identifier );
+ try testing.expectEqual(tags[ 2], .equal );
+ try testing.expectEqual(tags[ 3], .builtin );
+ try testing.expectEqual(tags[ 4], .l_paren );
+ try testing.expectEqual(tags[ 5], .string_literal);
+ try testing.expectEqual(tags[ 6], .r_paren );
+ try testing.expectEqual(tags[ 7], .semicolon );
+ try testing.expectEqual(tags[ 8], .keyword_pub );
+ try testing.expectEqual(tags[ 9], .keyword_fn );
+ try testing.expectEqual(tags[10], .identifier );
+ try testing.expectEqual(tags[11], .l_paren );
+ try testing.expectEqual(tags[12], .r_paren );
+ try testing.expectEqual(tags[13], .identifier );
+ try testing.expectEqual(tags[14], .bang );
+ try testing.expectEqual(tags[15], .identifier );
+ try testing.expectEqual(tags[16], .l_brace );
+ try testing.expectEqual(tags[17], .identifier );
+ try testing.expectEqual(tags[18], .period );
+ try testing.expectEqual(tags[19], .identifier );
+ try testing.expectEqual(tags[20], .period );
+ try testing.expectEqual(tags[21], .identifier );
+ try testing.expectEqual(tags[22], .l_paren );
try testing.expectEqual(tags[23], .string_literal);
- try testing.expectEqual(tags[24], .comma);
- try testing.expectEqual(tags[25], .period);
- try testing.expectEqual(tags[26], .l_brace);
- try testing.expectEqual(tags[27], .r_brace);
- try testing.expectEqual(tags[28], .r_paren);
- try testing.expectEqual(tags[29], .semicolon);
- try testing.expectEqual(tags[30], .r_brace);
- try testing.expectEqual(tags[31], .eof);
+ try testing.expectEqual(tags[24], .comma );
+ try testing.expectEqual(tags[25], .period );
+ try testing.expectEqual(tags[26], .l_brace );
+ try testing.expectEqual(tags[27], .r_brace );
+ try testing.expectEqual(tags[28], .r_paren );
+ try testing.expectEqual(tags[29], .semicolon );
+ try testing.expectEqual(tags[30], .r_brace );
+ try testing.expectEqual(tags[31], .eof );
}
test "ensure capacity on empty list" {
@@ -769,23 +769,23 @@ test "ensure capacity on empty list" {
list.appendAssumeCapacity(.{ .a = 3, .b = 4 });
try testing.expectEqualSlices(u32, &[_]u32{ 1, 3 }, list.items(.a));
- try testing.expectEqualSlices(u8, &[_]u8{ 2, 4 }, list.items(.b));
+ try testing.expectEqualSlices(u8 , &[_]u8 { 2, 4 }, list.items(.b));
list.len = 0;
list.appendAssumeCapacity(.{ .a = 5, .b = 6 });
list.appendAssumeCapacity(.{ .a = 7, .b = 8 });
try testing.expectEqualSlices(u32, &[_]u32{ 5, 7 }, list.items(.a));
- try testing.expectEqualSlices(u8, &[_]u8{ 6, 8 }, list.items(.b));
+ try testing.expectEqualSlices(u8 , &[_]u8 { 6, 8 }, list.items(.b));
list.len = 0;
try list.ensureTotalCapacity(ally, 16);
- list.appendAssumeCapacity(.{ .a = 9, .b = 10 });
+ list.appendAssumeCapacity(.{ .a = 9, .b = 10 });
list.appendAssumeCapacity(.{ .a = 11, .b = 12 });
- try testing.expectEqualSlices(u32, &[_]u32{ 9, 11 }, list.items(.a));
- try testing.expectEqualSlices(u8, &[_]u8{ 10, 12 }, list.items(.b));
+ try testing.expectEqualSlices(u32, &[_]u32{ 9, 11 }, list.items(.a));
+ try testing.expectEqualSlices(u8 , &[_]u8 { 10, 12 }, list.items(.b));
}
test "insert elements" {
@@ -803,7 +803,7 @@ test "insert elements" {
try list.ensureUnusedCapacity(ally, 1);
list.insertAssumeCapacity(1, .{ .a = 2, .b = 3 });
- try testing.expectEqualSlices(u8, &[_]u8{ 1, 2 }, list.items(.a));
+ try testing.expectEqualSlices(u8 , &[_]u8 { 1, 2 }, list.items(.a));
try testing.expectEqualSlices(u32, &[_]u32{ 2, 3 }, list.items(.b));
}
@@ -895,7 +895,7 @@ test "sorting a span" {
var n: u32 = 3;
for (sliced.items(.chr)[i..j], sliced.items(.score)[i..j]) |chr, score| {
try testing.expectEqual(score, n);
- try testing.expectEqual(chr, c);
+ try testing.expectEqual(chr , c);
n += 1;
}
c += 1;
presentfactory
andrewrk
nissarin
rohlem
andrewjradcliffe
kingofknights
After working with Zig for several years, I want to propose to reverse the current auto-format stance on alignment.
I don't think anyone disagrees that readability is improved when certain things are more aligned, especially when it's tables of data. We can find tables of data in plenty of real world projects. Furthermore, it has become common to use
zig fmt: offin many switch expressions, a pattern that certainly helps avoid bugs.My arguments against alignment were primarily that it harms line-based text diffs and conflict resolution since a single-line change may affect more than one line. Secondarily, that the auto-formatter may not have enough information to make a reasonable choice on when or how to align things.
Regarding the first point, I think that Zig should look towards the future rather than status quo. Will we be stuck with contemporary version control software forever? Perhaps not. Modern tools such as git already support ignoring whitespace changes, both in viewing diffs and in doing conflict resolution. Future tools may offer something beyond text diffs, so let's not let that hold us down, and in fact let's provide some incentive to innovate. I think the value of alignment is more than I previously estimated, and the harm of line diff noise less than I previously estimated.
As for when or how to align things, I have a concrete algorithm to propose:
What about unicode? Two options here:
Optional extension to this proposal:
// zig fmt: aligncould be used to additionally opt-in to more aggressive alignment for any particular line group which would do alignment even if the list of token tags were not identical.I think if we want to proceed with this, we should start by implementing the algorithm in zig fmt, run it on some real world codebases, and then see if the maintainers of those projects find the changes favorable.
Related:
2196