[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

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