[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

llunak abandoned this revision. llunak added a comment. This review did make sense when it was created, it's just that it took so long to get this processed that it was simpler to revert the original patch. Anyway, I've merged the change to https://reviews.llvm.org/D69778 . Repository: rC Cl

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

dblaikie added a comment. Yeah, I was confused by all this too - @aganea's right - this review doesn't make sense to me, since it doesn't show a proposed change to LLVM, it shows a proposed change on top of another patch, that would necessarily be committed together/in a single commit (a propos

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

dblaikie added a comment. (updating the previous review has the advantage that reviewers can review the diff relative to the previous version of the patch, using Phabricator's "History" feature, so they can review the delta since the committed/reverted version) Repository: rC Clang CHANGES

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

aganea added a comment. To ease reviewers comprehension you have to propose patches that diff against the master. People sometimes apply the patches locally before accepting them. This current patch is a subtraction after D69778 is applied locally on master. It

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

aganea added a comment. Once you do that, maybe you could send another message to the cfe-dev mailing list to ask for reviewers who have interest into optimizing PCH? I do, but I don't have the deep knowledge of this code. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

llunak added a comment. What is the practical difference? Either way the end result will be the same. And given that I get to wait ages for reviews of my PCH changes I find it more likely to get a review for a small patch rather than a large one. Repository: rC Clang CHANGES SINCE LAST ACTI

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

aganea added a comment. My point is that all the changes in this patch are already in the upstream. If you want to re-land D69778 , you should rather re-open and update that patch instead? (and close this current one) Repository: rC Clang CHANGES SINCE LAST

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

llunak added a comment. @aganea: This change reverts only a small part of my previous change (see my comment here from Feb 23). Hans reverted the original change only until this problem gets sorted out (see his comment from Feb 27 right before the revert). Repository: rC Clang CHANGES SINCE

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

aganea added a comment. @llunak : It seems @hans already reverted this in rG7ea9a6e0220da36ff2fd1fbc29c2755be23e5166 , could you please ensure this patch is still relevant? Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

aganea added a comment. If I understand correctly, this patch reverts the behavior to what it was before your changes, to only handle modules, but not PCH? Is that correct? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74846/new/ https://reviews.llvm.org/D74846

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

llunak added a comment. Ping 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

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

llunak added a comment. Ping... 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/

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

llunak added a comment. Ping.. 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/l

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

llunak added a comment. Ping. I don't particularly care about the declspec(selectany) corner case, but at least the mistake from D69778 should be fixed (and it's a simple fix), so that it can be committed again. Repository: rC Clang CHANGES SINCE LAST ACTI

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

hans added a comment. In D74846#1895319 , @aganea wrote: > Thanks @hans! Nevertheless I think this is a good change and we should push > it for 11.0. I was indeed seeing better timings on our end with what @llunak > was proposing, we are heavy users of P

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

aganea added a comment. Thanks @hans! Nevertheless I think this is a good change and we should push it for 11.0. I was indeed seeing better timings on our end with what @llunak was proposing, we are heavy users of PCH headers (and migrating to modules is not trivial in our case, lots of code br

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

hans added a comment. I've reverted cbc9d22e49b434b6ceb2eb94b67079d02e0a7b74 on master in 7ea9a6e0220da36ff2fd1fbc29c2755be23e5166 until it can be resolve

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

llunak added a comment. In D74846#1891580 , @dblaikie wrote: > In D74846#1891486 , @llunak wrote: > > > But before we get to this, can we please first fix my patch for 10.0? I > > didn't check properly for -fmodule

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

dblaikie added a comment. In D74846#1891486 , @llunak wrote: > In D74846#1889826 , @dblaikie wrote: > > > I know it's a bit of an awkward situation to test - but please include one > > to demonstrate why the origin

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

llunak added a comment. In 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

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

aganea added a comment. @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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74846/new/ https://reviews.llvm.org/D74

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

dblaikie added a comment. In D74846#1888410 , @llunak wrote: > Upon further investigation it turns out I probably should not have enabled > those two places for PCH in D69778 at all, > since they do not deal with -fmodul

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

llunak updated this revision to Diff 246122. llunak retitled this revision from "fix -fcodegen-modules code when used with PCH (PR44958)" to "fix -fcodegen-modules code when used with PCH (PR44953)". llunak edited the summary of this revision. llunak added a comment. Upon further investigation it