This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Consolidate tools and toolchains by target platform. (NFC)
ClosedPublic

Authored by dlj on Feb 24 2017, 9:27 PM.

Details

Summary

(This is a move-only refactoring patch. There are no functionality changes.)

This patch splits apart the Clang driver's tool and toolchain implementation
files. Each target platform toolchain is moved to its own file, along with the
closest-related tools. Each target platform toolchain has separate headers and
implementation files, so the hierarchy of classes is unchanged.

There are some remaining shared free functions, mostly from Tools.cpp. Several
of these move to their own architecture-specific files, similar to r296056. Some
of them are only used by a single target platform; since the tools and
toolchains are now together, some helpers now live in a platform-specific file.
The balance are helpers related to manipulating argument lists, so they are now
in a new file pair, CommonArgs.h and .cpp.

I've tried to cluster the code logically, which is fairly straightforward for
most of the target platforms and shared architectures. I think I've made
reasonable choices for these, as well as the various shared helpers; but of
course, I'm happy to hear feedback in the review.

There are some particular things I don't like about this patch, but haven't been
able to find a better overall solution. The first is the proliferation of files:
there are several files that are tiny because the toolchain is not very
different from its base (usually the Gnu tools/toolchain). I think this is
mostly a reflection of the true complexity, though, so it may not be "fixable"
in any reasonable sense. The second thing I don't like are the includes like
"../Something.h". I've avoided this largely by clustering into the current file
structure. However, a few of these includes remain, and in those cases it
doesn't make sense to me to sink an existing file any deeper.

Diff Detail

Repository
rL LLVM

Event Timeline

dlj created this revision.Feb 24 2017, 9:27 PM

Have you considered using "include_directories" in CMakeLists.txt to avoid including "../Something.h"?

dlj added a comment.Feb 26 2017, 11:48 PM

Have you considered using "include_directories" in CMakeLists.txt to avoid including "../Something.h"?

I don't want to take that approach, and there's a specific reason why: my concern isn't with three extra bytes, it's the fact that the headers are in an unusual place. It's typical to include headers from a subdirectory, or from the includes directory, but far less common to include from a parent lib directory. Hiding that fact seems strictly worse to me.

In D30372#687054, @dlj wrote:

Have you considered using "include_directories" in CMakeLists.txt to avoid including "../Something.h"?

I don't want to take that approach, and there's a specific reason why: my concern isn't with three extra bytes, it's the fact that the headers are in an unusual place. It's typical to include headers from a subdirectory, or from the includes directory, but far less common to include from a parent lib directory. Hiding that fact seems strictly worse to me.

OK. I was suggesting that only because LLVM already uses include_directories to include a header that exists in the parent directory in some cases. Apparently that's not what you wanted.

mehdi_amini edited edge metadata.Feb 27 2017, 10:15 PM
In D30372#687054, @dlj wrote:

Have you considered using "include_directories" in CMakeLists.txt to avoid including "../Something.h"?

I don't want to take that approach, and there's a specific reason why: my concern isn't with three extra bytes, it's the fact that the headers are in an unusual place. It's typical to include headers from a subdirectory, or from the includes directory, but far less common to include from a parent lib directory. Hiding that fact seems strictly worse to me.

OK. I was suggesting that only because LLVM already uses include_directories to include a header that exists in the parent directory in some cases. Apparently that's not what you wanted.

Yes it seems "common" in LLVM to do this in CMake: include_directories( ${CMAKE_CURRENT_BINARY_DIR}/.. ${CMAKE_CURRENT_SOURCE_DIR}/.. )

dlj added a comment.Feb 28 2017, 1:02 PM
In D30372#687054, @dlj wrote:

Have you considered using "include_directories" in CMakeLists.txt to avoid including "../Something.h"?

I don't want to take that approach, and there's a specific reason why: my concern isn't with three extra bytes, it's the fact that the headers are in an unusual place. It's typical to include headers from a subdirectory, or from the includes directory, but far less common to include from a parent lib directory. Hiding that fact seems strictly worse to me.

OK. I was suggesting that only because LLVM already uses include_directories to include a header that exists in the parent directory in some cases. Apparently that's not what you wanted.

Yes it seems "common" in LLVM to do this in CMake: include_directories( ${CMAKE_CURRENT_BINARY_DIR}/.. ${CMAKE_CURRENT_SOURCE_DIR}/.. )

Actually, since I'm still keeping these files in the clangDriver rule, the lib/Driver directory is already on the search path, so I can use a subpath of Driver already.

dlj updated this revision to Diff 90076.Feb 28 2017, 1:02 PM
  • Switch ../ paths to be relative to lib/Driver instead.
dlj updated this revision to Diff 90263.Mar 1 2017, 5:18 PM

Merged r296430 and r296485.

This revision was automatically updated to reflect the committed changes.