dexonsmith added a comment. I have a couple of drive-by suggestions, up to @dang and @Bigcheese whether to incorporate them.
================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:137-142 + .Case("static", llvm::Reloc::Static) + .Case("pic", llvm::Reloc::PIC_) + .Case("ropi", llvm::Reloc::ROPI) + .Case("rwpi", llvm::Reloc::RWPI) + .Case("ropi-rwpi", llvm::Reloc::ROPI_RWPI) + .Case("dynamic-no-pic", llvm::Reloc::DynamicNoPIC) ---------------- I wonder if it's worth creating a `.def` file for the driver enums, something like: ``` #ifdef HANDLE_RELOCATION_MODEL HANDLE_RELOCATION_MODEL("static", llvm::Reloc::Static) HANDLE_RELOCATION_MODEL("pic", llvm::Reloc::PIC_) HANDLE_RELOCATION_MODEL("ropi", llvm::Reloc::ROPI) HANDLE_RELOCATION_MODEL("rwpi", llvm::Reloc::RWPI) HANDLE_RELOCATION_MODEL("ropi-rwpio", llvm::Reloc::ROPI_RWPI) HANDLE_RELOCATION_MODEL("dynamic-no-pic", llvm::Reloc::DynamicNoPIC) #undef HANDLE_RELOCATION_MODEL #endif // HANDLE_RELOCATION_MODEL #ifdef HANDLE_DEBUG_INFO_KIND HANDLE_DEBUG_INFO_KIND("line-tables-only", codegenoptions::DebugLineTablesOnly) HANDLE_DEBUG_INFO_KIND("line-directives-only", codegenoptions::DebugDirectivesOnly) HANDLE_DEBUG_INFO_KIND("limited", codegenoptions::LimitedDebugInfo) HANDLE_DEBUG_INFO_KIND("standalone", codegenoptions::FullDebugInfo) #undef HANDLE_DEBUG_INFO_KIND #endif // HANDLE_DEBUG_INFO_KIND // ... ``` Then you can use `HANDLE_RELOCATION_MODEL` in both `normalize` and `denormalize`, rather than duplicating the table. Maybe we can go even further. Can you expand the `Values` array from the tablegen to include this info? Or rejigger the help text to leverage `HANDLE_RELOCATION_MODEL` (maybe pass in `ValuesDefine<HANDLE_RELOCATION_MODEL>`)? The current patch adds a value table; my first suggestion leaves us even; but maybe we can get rid of one. ================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3647 + DiagnosticsEngine &Diags) { +#define OPTION_WITH_MARSHALLING +#define OPTION_WITH_MARSHALLING_FLAG(PREFIX_TYPE, NAME, ID, KIND, GROUP, \ ---------------- Seems like `Options.inc` could provide this as a default definition, not sure if that seems error-prone? ================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3665-3667 +#undef OPTION_WITH_MARSHALLING_STRING +#undef OPTION_WITH_MARSHALLING_FLAG +#undef OPTION_WITH_MARSHALLING ---------------- I prefer the style where the `.inc` file is responsible for the `#undef` calls. It avoids having to duplicate it everywhere (and the risk of forgetting it). WDYT? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79796/new/ https://reviews.llvm.org/D79796 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits