Page MenuHomePhabricator

Change some series of AddAllArgs calls to use the most general AddAlllArgs.
ClosedPublic

Authored by dougk on Jul 29 2015, 9:00 AM.

Details

Summary

In a bunch of places where we had several calls to AddAllArgs, the new overload accepting an ArrayRef can be used.

But there is actually an observable difference: the old way re-ordered the rendered arguments across groups; but within a single group, they appear in-order as originally expressed.
The new behavior preserves original order throughout, and I assume that this is generally to be preferred.
It would be strange indeed if the driver was required to alter the order as written (at least not without a comment saying "We need to reorder these...").
Which is to say, I think that being faithful to what the user supplied should not be wrong.

Patch depends on http://reviews.llvm.org/D11597

Diff Detail

Event Timeline

dougk updated this revision to Diff 30914.Jul 29 2015, 9:00 AM
dougk retitled this revision from to Change some series of AddAllArgs calls to use the most general AddAlllArgs..
dougk updated this object.
dougk added reviewers: chandlerc, jyknight.
dougk added subscribers: cfe-commits, bkramer.
dougk added inline comments.Jul 29 2015, 9:02 AM
lib/Driver/Tools.cpp
6516

oops. an 'AddLastArg' got mixed in here.
But, it seems that most other places do add *all* of the 'e' args.
Which is right?

chandlerc accepted this revision.Jul 29 2015, 9:55 AM
chandlerc edited edge metadata.

Very much prefer preserving the original order.

We were already working around the reorderings by putting the D and U groups into a single call. =/ This approach seems *much* cleaner.

This revision is now accepted and ready to land.Jul 29 2015, 9:55 AM
This revision was automatically updated to reflect the committed changes.