Page MenuHomePhabricator

Rename Symbols.cpp from Host to Symbols/LocateSymbolFile.{h,cpp}
ClosedPublic

Authored by zturner on Feb 27 2019, 11:33 AM.

Details

Summary

After this change, there is only 1 remaining dependency in Host to Target.

While this change might seem like kind of a heavy hammer to solving the dependency problem, it makes more sense even conceptually to be in Symbols. While some of the specific places to search for symbol files might change depending on the Host, this is not inherently true in the same way that "accessing the file system" or "starting threads" is fundamentally dependent on the Host.

PDBs, for example, recently became a reality on non-Windows platforms, and it's theoretically possible that DSYMs could become a thing on non MacOSX platforms (maybe in a remote debugging scenario). Other types of symbol files, such as DWO, DWP, etc have never been tied to any Host platform anyway.

So I think this change both makes sense logically, and also improves the layering.

Event Timeline

zturner created this revision.Feb 27 2019, 11:33 AM
jingham accepted this revision.Feb 27 2019, 12:00 PM

This seems reasonable to me. Any Host implementation can live off source/Host/common/Symbols.cpp, and only needs to do anything if it wants to augment that behavior. Seen that way, it is not defining API's that are a required step in porting to a new platform. So the API definitions don't need to be specially called out as being required of a Host implementation. And the implementation is still in source/Host so it's clear that this does provide host specific functionality. So that seems fine to me.

This revision is now accepted and ready to land.Feb 27 2019, 12:00 PM
clayborg accepted this revision.Feb 27 2019, 1:11 PM
JDevlieghere accepted this revision.Feb 27 2019, 1:12 PM

Thanks Zachary! This LGTM.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2019, 1:44 PM