Page MenuHomePhabricator

Make ToolChain::SelectTool virtual.
ClosedPublic

Authored by dougk on Jun 4 2015, 8:25 AM.

Details

Summary

This allows ToolChain to decide if Clang is not the right C compiler.

Also, removed comment in Driver::ShouldUseClangCompiler implying that there was an opt-out ability at that point - there isn't.

Diff Detail

Repository
rL LLVM

Event Timeline

dougk updated this revision to Diff 27123.Jun 4 2015, 8:25 AM
dougk retitled this revision from to Make ToolChain::SelectTool virtual..
dougk updated this object.
dougk edited the test plan for this revision. (Show Details)
dougk added a reviewer: nlewycky.
dougk added a subscriber: Unknown Object (MLST).
chandlerc edited edge metadata.Jun 4 2015, 2:51 PM

Suggested some improvements to the comments below. Generally, this makes sense, but I think you should also mention your end-goal in the commit log. I happen to know because we've chatted, and so this change makes sense to me, but others won't have that context.

For those reading the review log, the context is that there exist some embedded platforms which use specialized C derivatives and C compilers where Clang doesn't support their derivative yet. We'd like to add support for these platforms to Clang, but at least until we do, its useful if the toolchain for such an embedded target can detect special flags and dispatch to whatever embedded compiler is commonly used. This makes the Clang driver more of a drop-in replacement and starts us down the path of support.

Actually, Doug, it might be worth sending an email to cfe-dev with the overall plan you have here to support C-dialects on embedded platforms by dispatching to the vendor-provided SDK until Clang grows support for that dialect. There you can give an overview and hopefully that'll provide context for folks seeing these commits.

chandlerc accepted this revision.Jun 9 2015, 1:22 AM
chandlerc edited edge metadata.

Folks on cfe-dev seem happy with this direction, so go ahead and submit with a tweaked comment as below...

include/clang/Driver/ToolChain.h
166–171 ↗(On Diff #27123)

Sorry, my inline comment didn't take the first time.

I feel like this comment can be more direct, and avoid the weird "virtualized" wording. How about something like this:

/// Choose a tool to handle the action \p JA.
///
/// This can be overridden when a particular ToolChain needs to use
/// a C compiler other than Clang.

I think going into Driver::ShouldUseClangCompiler is more appropriate for the change description than a comment, it isn't really useful documentation to the reader of the interface.

This revision is now accepted and ready to land.Jun 9 2015, 1:22 AM
This revision was automatically updated to reflect the committed changes.