new .d file parser for stage1 compiler #2182
Conversation
|
I think it's time to make enough progress on #1964 that it can apply to this pull request. I'd rather solve this in userland than add an optional cmake thing. Plus we need a self hosted implementation anyway. Why solve it twice? |
|
I will help you with this @mikdusan. Sorry, been real busy with the upcoming release! |
|
I haven't forgotten about this, will be working on a proof of concept soon that should clear up the path forward here. |
src/dep_tokenizer.hpp
Outdated
| @@ -0,0 +1,32 @@ | |||
| #ifndef ZIG_DEP_TOKENIZER_HPP | |||
There was a problem hiding this comment.
All this stuff should be in userland.h.
There was a problem hiding this comment.
done. I'm holding off pushing to PR branch until I resolve a windows test failure.
andrewrk
added
the
work in progress
- wip for ziglang#2046 - clang .d output must be created with `clang -MV` switch - implemented in Zig - hybridized for zig stage0 and stage1 - zig test src-self-hosted/dep_tokenizer.zig
|
@mikdusan let me know when this is ready for review |
|
ready for review |
andrewrk
removed
the
work in progress
src-self-hosted/dep_tokenizer.zig
Outdated
| }; | ||
|
|
||
| // stage1 compiler support | ||
| var stage2_da = std.heap.DirectAllocator.init(); |
There was a problem hiding this comment.
this should use std.heap.c_allocator
src-self-hosted/dep_tokenizer.zig
Outdated
| var stage2_da = std.heap.DirectAllocator.init(); | ||
|
|
||
| export fn stage2_DepTokenizer_init(input: [*]const u8, len: usize) stage2_DepTokenizer { | ||
| const t = stage2_da.allocator.create(Tokenizer) catch unreachable; |
There was a problem hiding this comment.
Please don't invoke undefined behavior when out of memory occurs. You can panic - that's what stage1 currently does on OOM.
src-self-hosted/dep_tokenizer.zig
Outdated
| const otoken = self.handle.next() catch { | ||
| return stage2_DepNextResult{ | ||
| .ent = 0, | ||
| .textz = (std.Buffer.init(&self.handle.arena.allocator, self.handle.error_text) catch unreachable).toSlice().ptr, |
There was a problem hiding this comment.
catch unreachable is not justified here
There was a problem hiding this comment.
please clarify; is that to mean instead use @Panic() or to mean coding around it and return an error with some kind of hardcoded text?
There was a problem hiding this comment.
@panic is fine - that's what stage1 currently does on OOM. But catch unreachable invokes undefined behavior in release builds. We definitely don't want that since we can't guarantee that memory allocation will always succeed.
src-self-hosted/dep_tokenizer.zig
Outdated
| }; | ||
| return stage2_DepNextResult{ | ||
| .ent = @enumToInt(token.id) + u8(2), | ||
| .textz = (std.Buffer.init(&self.handle.arena.allocator, token.bytes) catch unreachable).toSlice().ptr, |
There was a problem hiding this comment.
catch unreachable is not justified here
src-self-hosted/dep_tokenizer.zig
Outdated
| }; | ||
|
|
||
| export const stage2_DepNextResult = extern struct { | ||
| // 0=error, 1=null, 2=token=target, 3=token=prereq |
There was a problem hiding this comment.
why not model this with an extern enum?
- use std.heap.c_allocator - use @Panic instead of unreachable - use extern enum for tokenizer result type
|
all the review requests are implemented. should i just hit the "resolve conversation" buttons? |
either way, I'll have another look! |
|
Well I know that was a wild ride. Thanks for your hard work! |
PR is not yet ready; placing here for review/feedback
wip for the first line of .d files is ambiguous due to clang's output format #2046
output must be created with
clang -MVswitchan optional unit test is currently avilable:
cmake -DZIG_BUILD_TEST_DEP_TOKENIZER=1 ...
make ... install
cd $ZIGSRC
zig test test/standalone/issue_2046/test_dep_tokenizer.zig