[PATCH] D94472: [WIP][clang][cli] Command line round-trip for HeaderSearch options

Mon Jan 25 11:45:44 PST 2021

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