llunak added a comment. In D74846#1889826 <https://reviews.llvm.org/D74846#1889826>, @dblaikie wrote:
> I know it's a bit of an awkward situation to test - but please include one to > demonstrate why the original code was inappropriate. At least useful for > reviewing/validating the patch, but also might help someone else not to make > the same change again in the future. I don't know how to do that, except by doing the does-not-crash test that you refused above. The only way I can trigger the change in ASTDeclWriter::VisitVarDecl() to make any difference is by using the _declspec(selectany), but that crashes without the fix and does nothing with it. I've tried static variables, static const variables, templated statics, inline variables, statics in functions and I don't know what the code actually affects other than _declspec(selectany) crashing. Also, since I apparently don't know what the code in fact causes or why _declspec(selectany) crashes, I actually also don't know why the original code was inappropriate, other than that it crashes for an unknown reason. Since I simply reused the modules code for PCH, maybe _declspec(selectany) would crash with modules too? I don't know. But before we get to this, can we please first fix my patch for 10.0? I didn't check properly for -fmodules-codegen in D69778 <https://reviews.llvm.org/D69778>, that's for certain. That can be fixed before finding out why exactly it causes the trouble it causes. In D74846#1891208 <https://reviews.llvm.org/D74846#1891208>, @aganea wrote: > @llunak Do you have time to address this patch this week? If not, I can > finish it and land it. Please let me know. We'd like to see it merged into > 10.0. Feel free to do whatever you think would help you. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74846/new/ https://reviews.llvm.org/D74846 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
