Page MenuHomePhabricator

[clang][cli] Port LangOpts option flags to new option parsing system
ClosedPublic

Authored by jansvoboda11 on Jul 16 2020, 12:34 PM.

Diff Detail

Event Timeline

dang created this revision.Jul 16 2020, 12:34 PM
jdoerfert resigned from this revision.Jul 16 2020, 2:54 PM
dang updated this revision to Diff 278729.Jul 17 2020, 5:11 AM

Fix formatting

jansvoboda11 commandeered this revision.Nov 30 2020, 3:39 AM
jansvoboda11 added a reviewer: dang.
jansvoboda11 added a subscriber: jansvoboda11.

Taking over this patch, as Daniel is no longer involved.

jansvoboda11 retitled this revision from Port LangOpts option flags to new option parsing system to [clang][cli] Port LangOpts option flags to new option parsing system.Nov 30 2020, 3:40 AM
jansvoboda11 edited the summary of this revision. (Show Details)
jansvoboda11 added a reviewer: dexonsmith.

Rebase, move options back, port new options

Fix formatting

Revert accidental changes

dexonsmith requested changes to this revision.Nov 30 2020, 5:25 PM

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
3314

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?

4019

Similarly I think a grep for def.*no_ would turn this one up (probably the rest too).

This revision now requires changes to proceed.Nov 30 2020, 5:25 PM

Rebase, migrate more options, switch to using BoolOption

jansvoboda11 edited the summary of this revision. (Show Details)Dec 11 2020, 4:27 AM

This is now ready for review.

dexonsmith requested changes to this revision.Dec 11 2020, 2:37 PM
dexonsmith added inline comments.
clang/include/clang/Driver/Options.td
1296–1313

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
302

These nits might be better to do in a follow-up, which also updated extractForwardValue, but since I'm seeing it now:

  • Should this use std::move?
  • Can we drop the KeyPath name?
template <typename T, typename U>
static T mergeForwardValue(T /*KeyPath*/, U Value) {
  return static_cast<T>(std::move(Value));
}
3632–3643

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?

This revision now requires changes to proceed.Dec 11 2020, 2:37 PM

Thanks for catching these!

clang/include/clang/Driver/Options.td
1296–1313

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
302

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?
To me, commenting the name out seems to only add unnecessary syntax, and dropping it entirely makes the signature less clear.

3632–3643

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.

Revert exceptions flags

Revert ObjCAutoRefCount option

dexonsmith accepted this revision.Dec 14 2020, 10:29 AM

This LGTM now.

clang/lib/Frontend/CompilerInvocation.cpp
302

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.

3632–3643

Okay, that sounds good to me.

This revision is now accepted and ready to land.Dec 14 2020, 10:29 AM
This revision was landed with ongoing or failed builds.Dec 16 2020, 1:17 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.
clang/include/clang/Driver/Options.td
1001

ChangedBy<NegFlag, and ResetBy<PosFlag> are a bit verbose. How about a shorthand for this most common form?