This is an archive of the discontinued LLVM Phabricator instance.

[NFC][clang] Move much of the argument handling code from Driver::BuildActions to Driver::handleArguments.
ClosedPublic

Authored by plotfi on Aug 10 2019, 6:02 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

plotfi created this revision.Aug 10 2019, 6:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2019, 6:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
plotfi updated this revision to Diff 214545.Aug 10 2019, 6:04 PM

really sorry for how bad this diff looks. phab just doesnt present in a very ideal way.

aaron.ballman added inline comments.Aug 13 2019, 6:19 AM
clang/include/clang/Driver/Driver.h
264 ↗(On Diff #214545)

I think the presence of the YcArg and YuArg parameters needs some explanation in the comments, as those seem rather out of place.

plotfi marked an inline comment as done.Aug 13 2019, 11:45 AM
plotfi added inline comments.
clang/include/clang/Driver/Driver.h
264 ↗(On Diff #214545)

Those are the only corner case as far as things used and modified in the scope. Was just trying to preserve the existing behavior. I think those two args are used for clang-cl and the code that handles them just sets them to nullptr. I can add a comment trying to explain what is happening here. There were some tests that I remember were failing if those args weren't set to nullptr after being used.

plotfi updated this revision to Diff 214894.Aug 13 2019, 12:02 PM

Moved the initialization of YcArg and YuArg and initial handling of those args into handleArguments. ninja check-clang passes.

plotfi marked 2 inline comments as done.Aug 13 2019, 12:04 PM
plotfi added inline comments.
clang/include/clang/Driver/Driver.h
264 ↗(On Diff #214545)

I got rid of those arguments. Moved the code that starts setting those and using those variables into handleArguments. That makes sense to me since that code is handling the clang-cl /Yu and /Yc args.

plotfi updated this revision to Diff 214896.Aug 13 2019, 12:05 PM
plotfi updated this revision to Diff 214897.

@aaron.ballman Good catch on the /Yc /Yu args. I like the way its looking a lot more now.

Ugh, this is still not the most structured handling of the arguments. But, yeah, this seems like it should be equivalent. Fine by me if @aaron.ballman has no more comments.

This revision is now accepted and ready to land.Aug 14 2019, 6:10 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 10:01 AM