dexonsmith added a comment. Thanks for talking through this with me! The direction sounds good.
In D94472#2519838 <https://reviews.llvm.org/D94472#2519838>, @jansvoboda11 wrote: > I think the driver command line options could be useful even after we're > finished You mean the `-cc1` options used by the driver right? We shouldn't be adding options to the driver. I agree they should be kept. > Yes, this requires splitting `OPTIONS_WITH_MARSHALLING`. I don't think it's a > big deal, because we already do that for `DiagnosticOpts`, `LangOpts` and > `CodegenOpts` anyway (D84673 <https://reviews.llvm.org/D84673>, D94682 > <https://reviews.llvm.org/D94682>). I think it makes more sense to have one > parsing function per `*Opts`, rather than keeping it split up into > `Parse*Opts` and `ParseSimpleOpts`. Okay, that's a fair point; it probably really is cleaner to have one parse / generate function per option class. In that case, I wonder if we can remove the prefix from the key paths the same way as for `DiagnosticOpts`... >> 2. Boilerplate. Somewhat related to churn; there's a fair bit of additional >> boilerplate required in this approach. This makes it harder to read / >> understand / modify the code. > > Boilerplate was added mainly to `ArgList`, but we can delete that without > even showing up in `git blame`. Another instance is the lambda setup in > `ParseHeaderSearchArgs`, but I think that's pretty isolated as well. I agree > the code may be confusing for people not familiar with this effort, but it > will be me deleting the code few weeks from now, so I'm not sure how bad this > is. Do you worry this will cause confusion during merge conflicts down stream? Yes, it's merge conflicts that I think would be most confusing. I think you've convinced me that splitting up the parsers is independently valuable, so I'm less concerned about the churn now. > So people build without assertions during development? In that case, I agree > that erroring out on `GeneratedArgs1 != GeneratedArgs2` (in all kinds of > builds) would improve the experience. I don't think there's anything > preventing us to incorporate this into the current patch. Sounds great. This also checks that generation is deterministic. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94472/new/ https://reviews.llvm.org/D94472 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
