Page MenuHomePhabricator

Mostly standardize on "MumbleToolChain" as the ToolChain subclass name. NFC
ClosedPublic

Authored by dougk on Jun 22 2015, 11:38 AM.

Details

Summary

Variation in naming Hexagon_TC, NaCl_TC vs. TCEToolChain vs. the unadorned "XCore" imposes a stumbling block to understanding the clang driver. This patch changes the aforementioned that were suffixed with _TC to be suffixed with ToolChain.

Additionally, XCore is now XCoreToolChain because it was ambiguous with XCore as a namespace, unlike some others which were both a namespace and a ToolChain subclass, but could be distinguished by a difference in capitalization (solaris vs. Solaris, e.g.)

Diff Detail

Repository
rL LLVM

Event Timeline

dougk updated this revision to Diff 28139.Jun 22 2015, 11:38 AM
dougk retitled this revision from to Mostly standardize on "MumbleToolChain" as the ToolChain subclass name. NFC.
dougk updated this object.
dougk edited the test plan for this revision. (Show Details)
dougk added a subscriber: Unknown Object (MLST).
dougk updated this revision to Diff 28140.Jun 22 2015, 11:41 AM

Unconfused myself and added the right patch: nacltools is the namespace, NaClToolChain is the toolchain

dougk updated this revision to Diff 28142.Jun 22 2015, 11:51 AM

Re-touch comment that was poorly broken by clang-format-diff, really add all the right files.

dougk added subscribers: robertlytton, dschuff.

Adding the folks who originally authored the XCore and NaCl_TC as reviewers.

Since we're spelling out CrossWindowsToolChain, it shouldn't be much of an imposition to spell out NaClToolChain and XCoreToolChain

The content here seems fine, but clang-format has changed a lot of unrelated lines of code.

How about sending me just a fresh clang-format of lib/Driver? You'll want to look at the result and revert some hunks that don't make sense I suspect, but that would give you a much more consistent and clean baseline.

dschuff edited edge metadata.Jun 23 2015, 12:54 PM

renaming the classes for consistency LGTM; +1 for separating the unrelated formatting changes though.

chandlerc commandeered this revision.Jun 26 2015, 1:12 AM
chandlerc added a reviewer: dougk.

Please update once this is rebased on top of the formatted variant. I'd like to glance at it then before I stamp it. Thanks for the cleanup!!

OMG I hate the commandeer thing in Phab. I always think it will make me a reviewr. :: sigh ::

I go to try and fix this.

chandlerc edited reviewers, added: chandlerc; removed: robertlytton.Jun 26 2015, 1:13 AM
chandlerc edited edge metadata.

Oh great, I can't. Doug, sorry, just commandeer the revision back. Sorry i broke the review tool...

dougk commandeered this revision.Jun 26 2015, 12:29 PM
dougk removed a reviewer: dougk.

taking back

dougk updated this revision to Diff 28590.Jun 26 2015, 12:32 PM

Quite a bit less noise in the diff since the whitespace was fixed in a different patch.

Also renamed AssemblerARM to ARMAssembler.

rsmith accepted this revision.Jul 8 2015, 2:21 PM
rsmith added a reviewer: rsmith.
rsmith added a subscriber: rsmith.

LGTM

Chandler, do you still want to take another look at this?

lib/Driver/ToolChains.h
845–847 ↗(On Diff #28590)

These should be /// comments. Also, while you're touching this line, typo "the the". (And maybe commit this hunk separately.)

This revision is now accepted and ready to land.Jul 8 2015, 2:21 PM
This revision was automatically updated to reflect the committed changes.