This is an archive of the discontinued LLVM Phabricator instance.

[NFC][clang] Adding argument based Phase list filtering to getComplicationPhases
ClosedPublic

Authored by plotfi on Aug 8 2019, 10:01 PM.

Details

Summary

This patch removes usage of FinalPhase from anywhere outside of the scope where it is used to do argument handling.
It also adds argument based trimming of the Phase list pulled out of the Types.def table.

Diff Detail

Repository
rL LLVM

Event Timeline

plotfi created this revision.Aug 8 2019, 10:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 10:01 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Aug 9 2019, 6:45 AM
clang/lib/Driver/Driver.cpp
3220 ↗(On Diff #214294)

Why do you need this compound statement?

clang/lib/Driver/Types.cpp
313–315 ↗(On Diff #214294)

How about:

llvm::copy_if(PhaseList, std::back_inserter(P), [](phases::ID Phase) { return Phase <= phases.Preprocess; });

Similar below.

348–349 ↗(On Diff #214294)

P = PhaseList; ? Or does P come in with data already in it, in which case, llvm::copy() is a better choice.

plotfi updated this revision to Diff 214403.Aug 9 2019, 10:43 AM
plotfi marked 2 inline comments as done.
plotfi added inline comments.
clang/lib/Driver/Driver.cpp
3220 ↗(On Diff #214294)

I think the diff I posted here was a little bit off. I will update.

plotfi marked an inline comment as done.Aug 9 2019, 10:45 AM
plotfi added inline comments.
clang/lib/Driver/Types.cpp
313–315 ↗(On Diff #214294)

That could work. I was using llvm::remove_if before and ran into some problematic behavior.

plotfi updated this revision to Diff 214408.Aug 9 2019, 11:08 AM

using llvm::copy

plotfi marked 2 inline comments as done.Aug 9 2019, 11:09 AM
plotfi marked an inline comment as done.Aug 9 2019, 11:11 AM
plotfi added inline comments.
clang/lib/Driver/Driver.cpp
3220 ↗(On Diff #214408)

@aaron.ballman How's this look to you now? I have another NFC patch coming up that will move this entire block into its a separate function. I think finally this Drive::BuildActions code can have the code that handles and does things with various arguments separate from the actual job phase pipeline setup code.

aaron.ballman added inline comments.Aug 10 2019, 9:15 AM
clang/lib/Driver/Driver.cpp
3326 ↗(On Diff #214408)

PL.empty() instead

clang/lib/Driver/Types.cpp
11 ↗(On Diff #214408)

Can you sort these includes?

309 ↗(On Diff #214408)

You can elide braces for all of the if statement bodies here.

plotfi updated this revision to Diff 214541.Aug 10 2019, 2:06 PM
plotfi marked 5 inline comments as done.
plotfi added inline comments.
clang/lib/Driver/Types.cpp
11 ↗(On Diff #214408)

sorted.

309 ↗(On Diff #214408)

elided

compnerd added inline comments.Aug 10 2019, 9:31 PM
clang/include/clang/Driver/Types.h
107 ↗(On Diff #214541)

This really makes things confusing, perhaps renaming getCompilationPhases to getCompletePhaseList or something might make it less confusing? Although, I suppose that you do have follow up patches to improve this.

plotfi marked 4 inline comments as done.Aug 11 2019, 12:34 AM
plotfi added inline comments.
clang/include/clang/Driver/Types.h
107 ↗(On Diff #214541)

I'm mainly going by the original name. But I can certainly do this in a followup patch. Actually I'd prefer to not do it here, and address this in a patch where I remove the other getFinalPhase method from the Driver class.

plotfi marked an inline comment as done.Aug 12 2019, 6:44 PM

@aaron.ballman How does this look to you?

This revision is now accepted and ready to land.Aug 13 2019, 5:47 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 11:41 AM