Skip to content

Conversation

@Techatrix
Copy link
Contributor

I initially just wanted to simplify the code that handles the slicing syntax in AstGen.zig but then found two bugs related to the special Slicing By Length syntax:

This example will incorrectly trigger slicing by length even though no end index has been provided. It will crash the compiler.

test {
    var foo = "";
    _ = foo[0..][0.. :0];
}

In this example, the provided sentinel expression is never analyzed and no sentinel mismatch safety check is reported.

test {
    var foo: [2:0]u8 = .{ 1, 2 };
    _ = foo[0.. :1][0..2];
}

The PR that added the slicing by length special case has explicitly mentioned support for s[start..:sentinel][0..len] and s[start..:sentinal_1][0..len:sentinel_2] but appears to have never tested what happens if the first sentinel is invalid.

The diff as a whole is not very readable. I would recommend to look at the commits individually.

Here is a Benchmark of running ast-check on Sema.zig:

poop "build/stage4-fast/bin/zig ast-check src/Sema.zig" "build/stage4-sos-fast/bin/zig ast-check src/Sema.zig" -d 20000
Benchmark 1 (192 runs): build/stage4-fast/bin/zig ast-check src/Sema.zig
  measurement          mean ± σ            min … max           outliers         delta
  wall_time           104ms ± 1.76ms     101ms …  109ms          0 ( 0%)        0%
  peak_rss           77.7MB ± 48.1KB    77.4MB … 77.8MB         24 (13%)        0%
  cpu_cycles          340M  ± 5.09M      332M  …  357M           1 ( 1%)        0%
  instructions        516M  ± 11.3       516M  …  516M           0 ( 0%)        0%
  cache_references   1.41M  ± 63.2K     1.24M  … 1.56M           7 ( 4%)        0%
  cache_misses        354K  ± 34.8K      298K  …  416K           0 ( 0%)        0%
  branch_misses      4.75M  ± 3.17K     4.75M  … 4.76M           9 ( 5%)        0%
Benchmark 2 (194 runs): build/stage4-sos-fast/bin/zig ast-check src/Sema.zig
  measurement          mean ± σ            min … max           outliers         delta
  wall_time           103ms ± 2.37ms     101ms …  129ms          2 ( 1%)          -  0.7% ±  0.4%
  peak_rss           77.8MB ± 67.5KB    77.6MB … 77.8MB          0 ( 0%)          +  0.1% ±  0.0%
  cpu_cycles          337M  ± 6.85M      332M  …  411M           3 ( 2%)          -  0.7% ±  0.4%
  instructions        517M  ± 9.75       517M  …  517M           2 ( 1%)          +  0.1% ±  0.0%
  cache_references   1.42M  ± 58.4K     1.25M  … 1.86M           8 ( 4%)          +  0.6% ±  0.9%
  cache_misses        341K  ± 30.2K      294K  …  484K           1 ( 1%)        ⚡-  3.8% ±  1.8%
  branch_misses      4.73M  ± 3.05K     4.73M  … 4.76M           1 ( 1%)          -  0.4% ±  0.0%

example:
```zig
test {
	var foo = "";
	_ = foo[0..][0.. :0];
}
```

A `.slice_sentinel` ast node may not have an end index.
example:
```zig
test {
    var foo: [2:0]u8 = .{ 1, 2 };
    _ = foo[0.. :1][0..2];
}
```

A `.slice_open` ast node will not have a end index nor sentinel.
@andrewrk andrewrk merged commit 271452d into ziglang:master Dec 29, 2024
10 checks passed
@andrewrk
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants