src: mitigate MSVC dynamic initialization bug#25596
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the c++ refack commented Jan 20, 2019
addaleax commented Jan 20, 2019
The change LGTM, but as @seishun says in the other thread – it shouldn’t do anything at all? Along the lines of #25593 (comment): This shouldn’t be an issue when the definitions are all in one file, and it’s likely that something else is going on here… |
seishun commented Jan 20, 2019
Let's not make magical workarounds. We should figure out why this is happening and (most likely) submit a bug report to Visual Studio. |
refack commented Jan 20, 2019
I agree this is "magical". AFAICR that is why I didn't submit a PR with this till now. |
seishun commented Jan 20, 2019
If the underlying issue in the compiler is not fixed then it might break in another way in the next version. |
bzoz left a comment
bzoz left a comment There was a problem hiding this comment.
Until we get new VS with the bug fixed, we need to do something to make Node work again.
seishun commented Jan 22, 2019
FYI it works - just not the debug build. |
seishun commented Jan 22, 2019
In any case, the commit message should explain what the problem is and how it's being mitigated. |
seishun commented Jan 23, 2019
The problem is now "Under Investigation" so maybe it will be fixed very soon. |
addaleax commented Jan 23, 2019
@refack I think with an updated commit message this should be ready to go 👍 (And I’m assuming we want to merge this even if it gets fixed in the compiler itself) |
seishun commented Jan 23, 2019
I don't think so - I find it slightly more readable when an instance definition follows the class definition. |
I've tested |
refackforce-pushed the fix-static-order-windows branch from a7ad6a8 to 0c256f1Comparerefack changed the title refackforce-pushed the fix-static-order-windows branch from 0c256f1 to 31f4c4fComparePR-URL: nodejs#25596Fixes: nodejs#25593 Refs: https://developercommunity.visualstudio.com/content/problem/432157/dynamic-initializers-out-of-order.html Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
refackforce-pushed the fix-static-order-windows branch from 31f4c4f to e3c4b67Compare
PR-URL: #25596Fixes: #25593 Refs: https://developercommunity.visualstudio.com/content/problem/432157/dynamic-initializers-out-of-order.html Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
5 participants
Fixes: #25593
Refs: https://developercommunity.visualstudio.com/content/problem/432157/dynamic-initializers-out-of-order.html
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes