Fix check if nested function can access outer function frame#1888
Fix check if nested function can access outer function frame#1888kinke merged 1 commit intoldc-developers:masterfrom
Conversation
| fd = getParentFunc(fd, false); | ||
| assert(fd); | ||
| while (fd && fd != vdparent) { | ||
| fd = getParentFunc(fd); |
There was a problem hiding this comment.
fd will be null if fd is static (or a function literal) or if there are static aggregates in the parents hierarchy from fd up to the next outer function (if any, otherwise null as well). Note that this was the only place where stopOnStatic was false, apparently only to check for static-ness manually for the error, but unfortunately that false lead to static aggregate parents (and implicitly static function literals) being ignored.
|
The testcase is missing from the PR. |
|
It'd be a dmd-testsuite one (so added later), if it actually makes sense - I think it's really a front-end bug and should work, so I'm rather reluctant to add one. |
|
Whereever the bug is, we need a testcase for this. |
|
My point is that I'm reluctant to add an output-matching fail-compilation test for something that looks like it should be working. I.e., a test making sure that something produces an error (instead of crashing), although it looks like valid code. |
|
Ah, I misunderstood. So the problem is probably a rejects-valid type of bug? |
|
Probably, yes, I'm not sure. I can obviously add a test for the time being, so that we at least get notified when it works (if ever). The other thing wrt. static aggregate parents/function literals is probably hard/impossible to test, as I guess the front-end rejects the code earlier and we never get into a problematic |
|
The changes LGTM. I prefer a test for the time being, but leave it to you. |
The previous check wouldn't check intermediate aggregates for static-ness, that was one problem. The other was the assertion that the outer function can be reached as long as there are no static functions inbetween, which isn't always the case, as issue ldc-developers#1864 clearly shows, which is resolved by this.
|
I've added the test. |
CMake uses incremental linking for MSVC targets by default, with apparent effect on the alignment of globals. A bunch of unittests have been failing on Win64 without these fixes since letting CMake link the test runners. Rainer has fixed the 2 Phobos locations upstream already; the `core.thread` bug is still unfixed, I submitted PR ldc-developers#1888.
CMake uses incremental linking for MSVC targets by default, with apparent effect on the alignment of globals. A bunch of unittests have been failing on Win64 without these fixes since letting CMake link the test runners. Rainer has fixed the 2 Phobos locations upstream already; the `core.thread` bug is still unfixed, I submitted PR ldc-developers#1888.
CMake uses incremental linking for MSVC targets by default, with apparent effect on the alignment of globals. A bunch of unittests have been failing on Win64 without these fixes since letting CMake link the test runners. Rainer has fixed the 2 Phobos locations upstream already; the `core.thread` bug is still unfixed, I submitted PR ldc-developers#1888.
2 participants
The previous check wouldn't check intermediate aggregates for static-ness, that was one problem. The other was the assertion that the outer function can be reached as long as there are no static functions inbetween, which isn't always the case, as issue #1864 clearly shows, which is resolved by this.