This is an archive of the discontinued LLVM Phabricator instance.

Add methods to get archive file name from member file
Needs ReviewPublic

Authored by ruiu on Feb 5 2015, 2:02 PM.

Details

Reviewers
shankarke
Summary

Previously we only have File::path() to get the path name of a file.
If a file was a member of an archive file, path() returns a concatenated
string of the file name in the archive and the archive file name.
If we wanted to get a file name or an archive file name, we had to
parse that string. That's of course not good.

This patch adds new member functions, archivePath and memberPath, to File.

Diff Detail

Event Timeline

ruiu updated this revision to Diff 19431.Feb 5 2015, 2:02 PM
ruiu retitled this revision from to Add methods to get archive file name from member file.
ruiu updated this object.
ruiu edited the test plan for this revision. (Show Details)
ruiu added a reviewer: shankarke.
ruiu added a project: lld.
ruiu added a subscriber: Unknown Object (MLST).
shankarke edited edge metadata.Feb 5 2015, 2:13 PM

Other than the comment LGTM. Thanks for doing this.

include/lld/Core/File.h
65

Could we cache this path.

69

Spell error. Otehrwise -> otherwise.

ruiu added inline comments.Feb 5 2015, 2:15 PM
include/lld/Core/File.h
65

We could, but the cache needs to be invalidated when _archivePath is updated. This is used only for logging and debugging, I think we don't need to do that.

shankarke added inline comments.Feb 5 2015, 2:33 PM
include/lld/Core/File.h
65

I dont think we ever update the archivePath after setting it.

lib/ReaderWriter/FileArchive.cpp
206

There is still an issue that if there was a parse error, diagnostics will not show the proper error. Say the file was of a different architecture. I remember this was the reason I created ArchiveMemoryBuffer.

shankarke added inline comments.Feb 5 2015, 2:35 PM
lib/ReaderWriter/FileArchive.cpp
206

What I meant here, is that diagnostics will not show the full path of the file, where there was an error.

ruiu added inline comments.Feb 5 2015, 2:40 PM
lib/ReaderWriter/FileArchive.cpp
206

Oh, that's too minor and edge case to justify creating a new subclass of MemoryBuffer. If we want to include archive file name, we can simply returns a new error message here.

Sounds good, but I think the archive file can still be cacheable as there is nothing that invalidates the archive path once its set.