Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
When are you planning to refactor OptInFFlag to support the use cases called out in this patch? If it's right away / soon, I think it'd be a bit cleaner for to land the refactoring first, then the options that use it, as opposed to fixing them up later. WDYT? (If you're not planning to do it until "later", I don't feel strongly, but as noted inline I'm not sure we need all the todos.)
| clang/include/clang/Driver/Options.td | ||
|---|---|---|
| 2988 | I wonder if this TODO is helpful, or if you could find these later just by looking for def no_ thanks to the consistent naming that's used? | |
| 3689 | Similarly I think a grep for def.*no_ would turn this one up (probably the rest too). | |
| clang/include/clang/Driver/Options.td | ||
|---|---|---|
| 1069–1081 | These options should be mutually exclusive -- as in, the last flag wins -- but I don't see how that's implemented now (the previous logic was via getLastArg). If that is working properly, can you explain how? If it's not working right now, my suggestion would be to separate these options out to do in a separate patch series. I would suggest, rather than modelling the current behaviour, we leverage our flexibility to change -cc1 options (e.g., could do three patches, where the first adds accessors to LangOpts and updates all users, the second changes the keypath to a single ExceptionStyle enum, and then the third patch changes the -cc1 option to -fexception-style and starts using the marshalling infrastructure). | |
| clang/lib/Frontend/CompilerInvocation.cpp | ||
| 292 | These nits might be better to do in a follow-up, which also updated extractForwardValue, but since I'm seeing it now:
template <typename T, typename U>
static T mergeForwardValue(T /*KeyPath*/, U Value) {
return static_cast<T>(std::move(Value));
} | |
| 3620–3631 | Previously, these arguments were only claimed depending on -x; are we changing to claim these all the time? If so, that should be considered separately; I think in general we may want the ability to do claim some options only conditionally; @Bigcheese , any thoughts here? | |
Thanks for catching these!
| clang/include/clang/Driver/Options.td | ||
|---|---|---|
| 1069–1081 | Nice catch, thanks! I'll revert these changes and tweak the -cc1 command-line in a follow-up. Another option would be to keep these changes and check the exclusivity in FixupInvocation, but I prefer the enum. | |
| clang/lib/Frontend/CompilerInvocation.cpp | ||
| 292 | Adding std::move here and in extractForwardValue makes sense to me, I can do that in a follow-up. May I ask why are you so keen on dropping names of unused parameters? | |
| 3620–3631 | LangOpts.PIE is unconditionally populated at line 2901, so I think removing it here is fine. You're right about LangOpts.ObjCAutoRefCount, though. I think keeping the semantics the same is preferable, even though all tests pass even after this change. (It has also been removed at line 2547 which also doesn't seem right.) I'll revert this for now and come back to it when we land ShouldParseIf in D84674. | |
This LGTM now.
| clang/lib/Frontend/CompilerInvocation.cpp | ||
|---|---|---|
| 292 | Dropping unused parameter names makes it clear to the reader that it's intentional that the parameter is unused (for longer functions, it can also clarify that it's not used). If there were a forward declaration for this, I think it would make sense to have the parameter name in the forward declaration (to document what it's for) and drop it entirely in the definition (to document that it's skipped intentionally), but since there's no forward declaration I think commenting it out is a good compromise. | |
| 3620–3631 | Okay, that sounds good to me. | |
| clang/include/clang/Driver/Options.td | ||
|---|---|---|
| 804 | ChangedBy<NegFlag, and ResetBy<PosFlag> are a bit verbose. How about a shorthand for this most common form? | |
clang-format not found in user's PATH; not linting file.