dang marked 3 inline comments as done.
dang added inline comments.
================
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)
----------------
dexonsmith wrote:
> 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.
I think this suggestion, I can definitely do at least this to generate the
necessary switch statements. I don't see why this should be a separate .def
file, I can generate this from the tablegen with an extra field named
`ValuesAssociatedDefinitions` or something to that effect. I'll do that now.
================
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,
\
----------------
dexonsmith wrote:
> Seems like `Options.inc` could provide this as a default definition, not sure
> if that seems error-prone?
Not sure how much of this do you mean? `OPTION_WITH_MARSHALLING` is a
convenience thing that forces users to opt into getting the marshalling
definitions. I think it might be better to provide default empty definitions
for `OPTION_WITH_MARSHALLING_FLAG` and friends to achieve a similar effect
without needing `OPTION_WITH_MARSHALLING`. I you mean the actual definitions
here they rely on the `ArgList &` to be named Args which might make it error
prone to include these as defaults.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3665-3667
+#undef OPTION_WITH_MARSHALLING_STRING
+#undef OPTION_WITH_MARSHALLING_FLAG
+#undef OPTION_WITH_MARSHALLING
----------------
dexonsmith wrote:
> 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?
I prefer it too, but the current tablegen backend doesn't generate them for
other macros it defines, so I wanted to make it consistent... I could change
the existing backend to work that way but I would then need to track all the
usages of this across all the llvm-projects which I don't fancy doing. Let me
know if you thing it is fine to break with the existing style for this one.
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
