-
-
Notifications
You must be signed in to change notification settings - Fork 268
Fix issue 4130 #4135
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
Fix issue 4130 #4135
Conversation
@@ -307,7 +307,8 @@ LLValue *DtoNestedContext(const Loc &loc, Dsymbol *sym) { | |||
IF_LOG Logger::println("Current function is %s", ctxfd->toChars()); | |||
if (fromParent) { | |||
ctxfd = getParentFunc(ctxfd); | |||
assert(ctxfd && "Context from outer function, but no outer function?"); | |||
if (!ctxfd) | |||
ctxfd = irFunc.decl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add log output about this strange case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this actually produce correct code now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't crash, which is what it did before. As for the correctness of the code,I'm not really sure, but I don't know what its supposed to do other than iterate through the linked list of classes. @ZILtoid1991 could you provide a reduced test case of this code in action?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class DOMImplementation {
class Attr { Attr _nextAttr; }
class Map {
Attr firstAttr;
auto opSlice()
{
struct Range
{
Attr currentAttr;
auto front() { return currentAttr; }
void popFront() { currentAttr = currentAttr._nextAttr; }
bool empty() { return currentAttr is null; }
}
return Range(firstAttr);
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, somewhat further reduced:
class Outer {
class Inner {
auto opSlice() {
struct Range {
void foo() {}
}
return Range();
}
}
}
2 outer aggregates are apparently required, and both need to be classes (works with any one of them being a struct).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't crash, which is what it did before. As for the correctness of the code,I'm not really sure, but I don't know what its supposed to do other than iterate through the linked list of classes.
If unsure about the correct behavior, then better to exit gracefully instead of silently continuing.
579a056
to
4c8c396
Compare
4c8c396
to
42b8bb6
Compare
This fixes the issue, I'm not sure if the correct thing to do is to make DMD not emit bogus ASTs to begin with.
fixes #4130