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

to_cpp1: Remove missing initializer assert #1124

Closed
wants to merge 1 commit into from

Conversation

sookach
Copy link
Contributor

@sookach sookach commented Jun 18, 2024

Patches #1123

The program is ill formed, and semantic analysis detects the issue, but because errors are checked after lowering, it hits an assert when spitting out the cpp code, causing the crash. We could, of course, check for errors generated by semantic analysis before lowering, but this pr simply swaps out the assert for a return, so as to maintain the current structure.

@@ -6719,7 +6719,9 @@ class cppfront

// Emit "auto" for deduced types (of course)
if (type->is_wildcard()) {
assert(n.initializer);
if(!n.initializer) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be an error recorded here, like at line 6735?

Copy link
Contributor Author

@sookach sookach Jun 18, 2024

Choose a reason for hiding this comment

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

there would be a double reporting of the error since sema already records it.

cppfront/source/sema.h

Lines 1526 to 1529 in 442d17c

errors.emplace_back(
n.position(),
"an object with a deduced type must have an = initializer"
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good eye though

@sookach sookach requested a review from gregmarr June 18, 2024 22:12
@hsutter
Copy link
Owner

hsutter commented Jun 20, 2024

Thanks! I tried to push an alternate solution to this branch but I don't have permission, so I'll apply the solution on main separately instead of taking this PR, and reference this PR. Again, thanks for this.

@hsutter hsutter closed this in be9d805 Jun 20, 2024
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.

3 participants