Use std.Buffer even less #4665

daurnimator · 2020-03-07T05:06:58Z

Followup to #4405

andrewrk

Thanks, I do think these are some nice changes. Some adjustments to make and then it's good to merge.

lib/std/child_process.zig

lib/std/target.zig

andrewrk · 2020-03-12T17:49:31Z

lib/std/target.zig

@@ -967,23 +967,23 @@ pub const Target = struct {

    pub const stack_align = 16;

-    pub fn zigTriple(self: Target, allocator: *mem.Allocator) ![:0]u8 {
+    pub fn zigTriple(self: Target, allocator: *mem.Allocator) ![]u8 {


👍 These I agree to change

andrewrk

almost there

andrewrk · 2020-03-29T14:02:05Z

src/codegen.cpp

-        cache_str(ch, g->libc->include_dir);
-        cache_str(ch, g->libc->sys_include_dir);
-        cache_str(ch, g->libc->crt_dir);
-        cache_str(ch, g->libc->msvc_lib_dir);
-        cache_str(ch, g->libc->kernel32_lib_dir);


These need the separator, otherwise a/b/ c/d would hash the same as a/b /c/d.

ooo interesting. that that mean that cache_mem should also mix in the length?

I believe cache_mem is used internally as "put these bytes into the hash", whereas the public API functions are more along the lines of "put this thing in the hash and make sure it's separate"

Then perhaps that belongs in the cache_slice function below and hence answers you question there :)

andrewrk · 2020-03-29T14:03:06Z

src/cache_hash.cpp

+void cache_slice(CacheHash *ch, Slice<const char> slice) {
+    cache_mem(ch, slice.ptr, slice.len);
+}
+


do we need this API? why not use cache_mem directly?

andrewrk · 2020-03-29T14:05:34Z

src-self-hosted/libc_installation.zig

+            var result_buf = std.ArrayList(u8).init(allocator);
+            defer result_buf.deinit();


This is less efficient use of memory here. Before it only made one byte buffer and resized it in the loop; now it allocates and frees a fresh one every loop iteration. Why not port the code directly using ArrayList?

Depends on your allocator

For smart allocators that use a freelist, they will essentially do this automatically

For dumber allocator, each time through the loop may have a different sized allocation and they probably don't do realloc efficiently, so they end up doing multiple allocations anyway!

In general I have a preference to reduce the scope of variables as much as possible.

src/link.cpp

src-self-hosted/libc_installation.zig

data-man · 2020-03-31T05:32:40Z

lib/std/array_list.zig

+                fn appendWrite(self: *Self, m: []const u8) !usize {
+                    try self.appendSlice(m);
+                    return m.len;
+                }
+
+                pub fn outStream(self: *Self) std.io.OutStream(*Self, error{OutOfMemory}, appendWrite) {
+                    return .{ .context = self };
+                }


It's nice addition!
But maybe add a new structure with all necessary functions?
Something like std.io.MemoryStream.

…ranslate_c.zig

…ying on zero termination

andrewrk · 2020-04-01T17:45:41Z

This is merged now, with additionally the following (breaking) changes:

    std.Buffer => std.ArrayListSentineled(u8, 0)
    
    This new name (and the fact that it is a function returning a type) will
    make it more clear which use cases are better suited for ArrayList and
    which are better suited for ArrayListSentineled.
    
    Also for consistency with ArrayList,
     * `append` => `appendSlice`
     * `appendByte` => `append`
    
    Thanks daurnimator for pointing out the confusion of std.Buffer.

daurnimator added the standard library This issue involves writing Zig code for the standard library. label Mar 7, 2020

daurnimator force-pushed the less-buffer branch 5 times, most recently from ce18696 to fe69332 Compare March 12, 2020 14:02

andrewrk requested changes Mar 12, 2020

View reviewed changes

daurnimator force-pushed the less-buffer branch 5 times, most recently from 823b73f to 87a8e15 Compare March 18, 2020 07:25

daurnimator requested a review from andrewrk March 18, 2020 12:17

daurnimator force-pushed the less-buffer branch from 87a8e15 to 71bdb2d Compare March 29, 2020 04:48

andrewrk requested changes Mar 29, 2020

View reviewed changes

daurnimator commented Mar 31, 2020

View reviewed changes

src-self-hosted/libc_installation.zig Outdated Show resolved Hide resolved

data-man reviewed Mar 31, 2020

View reviewed changes

daurnimator added 6 commits March 31, 2020 21:54

std: add std.ArrayList(u8).outStream()

edb47c9

std: don't return sentinel slices from cross_target functions

1c56e99

std: use std.ArrayList(u8) instead of std.Buffer in std/build.zig

55e1f32

std: use Buffer.outStream in std/child_process.zig

f552d58

std: use std.ArrayList(u8) instead of std.Buffer in src-self-hosted/t…

1fce9e1

…ranslate_c.zig

std: use std.ArrayList(u8).OutStream instead of std.Buffer.OutStream

e3835e5

daurnimator force-pushed the less-buffer branch 2 times, most recently from 775d89a to 5ef2d0f Compare March 31, 2020 12:15

Use length field as passed in stage2 libc_installation instead of rel…

14fb7d2

…ying on zero termination

daurnimator force-pushed the less-buffer branch from 5ef2d0f to 14fb7d2 Compare March 31, 2020 14:22

andrewrk closed this in c2e8788 Apr 1, 2020

andrewrk added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Apr 1, 2020

daurnimator deleted the less-buffer branch April 1, 2020 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use std.Buffer even less #4665

Use std.Buffer even less #4665

daurnimator commented Mar 7, 2020

andrewrk left a comment

andrewrk Mar 12, 2020

andrewrk left a comment

andrewrk Mar 29, 2020

daurnimator Mar 31, 2020

andrewrk Mar 31, 2020

daurnimator Mar 31, 2020

andrewrk Mar 29, 2020

andrewrk Mar 29, 2020

daurnimator Mar 31, 2020

data-man Mar 31, 2020

andrewrk commented Apr 1, 2020

		var result_buf = std.ArrayList(u8).init(allocator);
		defer result_buf.deinit();

Use std.Buffer even less #4665

Use std.Buffer even less #4665

Conversation

daurnimator commented Mar 7, 2020

andrewrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk left a 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

andrewrk commented Apr 1, 2020