This is an archive of the discontinued LLVM Phabricator instance.

Split Target.cpp into small files.
ClosedPublic

Authored by ruiu on Jun 14 2017, 2:33 PM.

Details

Summary

Target.cpp contains code for all the targets that LLD supports. It was
simple and easy, but as the number of supported targets increased,
it got messy.

This patch splits the file into per-target files under ELF/arch directory.

Diff Detail

Repository
rL LLVM

Event Timeline

ruiu created this revision.Jun 14 2017, 2:33 PM

I like the idea here. My main concern is that I see several generic-looking functions in Target.h. Personally, I would kind of expect these functions to be in other locations, and Target.h only to include the interface that each of the different targets implements.

lld/ELF/Target.h
15 ↗(On Diff #102608)

Unused header?

18 ↗(On Diff #102608)

Unused header?

21 ↗(On Diff #102608)

This function is way too generically named to be in the LLD namespace. Perhaps something like relocationTypeToString. Also, it doesn't feel like it belongs in Target.h. Maybe Relocation.h?

118 ↗(On Diff #102608)

Should this be in a different file? Not sure where though on a quick glance.

127–130 ↗(On Diff #102608)

It feels to me like this and the other check* functions don't really belong in Target.h, since they're nothing to do with the target. It seems like they should really be in Relocations.h.

They're also marked as static, but in a header file, which looks wrong to me.

silvas added a subscriber: silvas.Jun 15 2017, 2:36 PM

Thanks, I like this idea. We might want to do something similar for the tests.

lld/ELF/CMakeLists.txt
10 ↗(On Diff #102608)

Sorry for such a stupid little comment, but it really should be Arch to be consistent with most LLVM directory naming conventions.

ruiu added inline comments.Jun 15 2017, 3:29 PM
lld/ELF/Target.h
15 ↗(On Diff #102608)

Done.

18 ↗(On Diff #102608)

Done.

21 ↗(On Diff #102608)

I agree that this is too generic, but since it's not new code, let's keep it as-is.

118 ↗(On Diff #102608)

Maybe. But let's not change too much in code-moving change.

127–130 ↗(On Diff #102608)

Ditto

ruiu updated this revision to Diff 102732.Jun 15 2017, 3:32 PM
  • Rename arch Arch
  • Address review comments.
ruiu added a comment.Jun 16 2017, 9:15 AM

Rafael, are you ok with this?

This revision was automatically updated to reflect the committed changes.