mstorsjo added a comment. In D126676#3687227 <https://reviews.llvm.org/D126676#3687227>, @dexonsmith wrote:
> I haven’t reviewed the details of the patch and won’t have time to do so (at > least for a while), but the description of the intended (more narrow) scope > SGTM. With the scope limited to GCC directories this should be fine. > > There are cases where it’s safe to have mismatched defines (e.g., if a define > can be proven during a cheap dependency scan not to matter for a given PCH, > it’s better to canonicalize away the define and share the artifact more > broadly) but if I understand correctly this patch won’t preclude compiler > smarts like that. Yup. Any implementation of such logic hasn’t materialized during the 10 years since todos hinting that we should implement that, but it doesn’t seem to be a big practical issue either, outside of false positive matches with gcc PCH directories - so I guess it’s not a high priority in practice. > If I’m right, then I think this can go in once @rnk is happy. (If I’m wrong, > maybe better to wait for one of the people I pinged.) Yeah, I’d be happy to add a validation level enum to make things a bit clearer - I’ll try to revise the patch soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126676/new/ https://reviews.llvm.org/D126676 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
