Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stage2: add compile log statement #7191

Merged
merged 13 commits into from
Dec 26, 2020
Merged

Conversation

g-w1
Copy link
Contributor

@g-w1 g-w1 commented Nov 22, 2020

The only problem that I ran into making this pr was that the output of the @compilelog can't be tested. There is no way to test compile-time output besides errors. Should this be added just to enable compile logging testing?

@daurnimator daurnimator added the frontend Tokenization, parsing, AstGen, Sema, and Liveness. label Nov 22, 2020
src/zir_sema.zig Outdated Show resolved Hide resolved
@g-w1
Copy link
Contributor Author

g-w1 commented Nov 23, 2020

As a temporary solution, Ill just make it print to stderr instead of using the mod.fail so that a decl can have other errors too.

@g-w1 g-w1 force-pushed the stage2-compile-log branch from 1db5e91 to 3a7c08e Compare November 27, 2020 22:39
@g-w1
Copy link
Contributor Author

g-w1 commented Nov 27, 2020

Just saw your comment on #7239 about this, the reason it was ok to just use ArrayListUnmanaged(usize) was because the error message for a @compileLog is just found compile log statement but for any failed_decl it would need to be ArrayListUnmanaged(*Compilation.ErrorMsg). Should I do this instead of what I have right now with compile_log_decls?

@g-w1
Copy link
Contributor Author

g-w1 commented Dec 14, 2020

Making this a draft until #7239 gets solved because it is very hard to debug this.

@g-w1 g-w1 marked this pull request as draft December 14, 2020 14:03
@g-w1 g-w1 force-pushed the stage2-compile-log branch from c62380a to 9f75473 Compare December 25, 2020 00:06
@g-w1 g-w1 marked this pull request as ready for review December 25, 2020 13:41
@g-w1
Copy link
Contributor Author

g-w1 commented Dec 25, 2020

The last commit fixes the memory leaks. They were in deleteDecl and markOutdatedDecl. This now closes #7218 and unblocks #7092.

@Vexu Vexu merged commit 1634d45 into ziglang:master Dec 26, 2020
self.bin_file.updateDecl(module, decl) catch |err| {
switch (err) {
error.OutOfMemory => return error.OutOfMemory,
error.AnalysisFail => {
decl.analysis = .dependency_failure;
},
else => {
try module.failed_decls.ensureCapacity(module.gpa, module.failed_decls.items().len + 1);
module.failed_decls.putAssumeCapacityNoClobber(decl, try ErrorMsg.create(
try module.addDeclErr(decl, try ErrorMsg.create(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every instance of this change introduced a memory leak if ErrorMsg.create fails.

andrewrk added a commit that referenced this pull request Dec 28, 2020
The addition of `addDeclErr` introduced a memory leak at every call
site, and I also would like to push back on having more than 1
compilation error per `Decl`.

This reverts commit 1634d45.
aarvay pushed a commit to aarvay/zig that referenced this pull request Jan 4, 2021
aarvay pushed a commit to aarvay/zig that referenced this pull request Jan 4, 2021
The addition of `addDeclErr` introduced a memory leak at every call
site, and I also would like to push back on having more than 1
compilation error per `Decl`.

This reverts commit 1634d45.
dgbuckley pushed a commit to dgbuckley/zig that referenced this pull request Mar 9, 2021
dgbuckley pushed a commit to dgbuckley/zig that referenced this pull request Mar 9, 2021
The addition of `addDeclErr` introduced a memory leak at every call
site, and I also would like to push back on having more than 1
compilation error per `Decl`.

This reverts commit 1634d45.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants