This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Sink StreamFile into lldbHost
ClosedPublic

Authored by bulbazord on Aug 8 2023, 5:22 PM.

Details

Summary

StreamFile subclasses Stream (from lldbUtility) and is backed by a File
(from lldbHost). It does not depend on anything from lldbCore or any of its
sibling libraries, so I think it makes sense for this to live in
lldbHost instead.

Diff Detail

Event Timeline

bulbazord created this revision.Aug 8 2023, 5:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 5:22 PM
bulbazord requested review of this revision.Aug 8 2023, 5:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 5:22 PM
mib added a comment.Aug 8 2023, 9:26 PM

Shouldn't this be next to the other Stream class in lldbUtility ?

lldb/include/lldb/Host/StreamFile.h
12

Do we really need this header ? We could forward declare FileSP do avoid having to include it here

Shouldn't this be next to the other Stream class in lldbUtility ?

lldbUtility is the lowest layer and does not depend on anything in lldbHost. That being said, StreamFile uses File and FileSystem from lldbHost, so that is the lowest layer it can sit on without a layering violation.

lldb/include/lldb/Host/StreamFile.h
12

We're actually using it in this header (look at GetFile()).

mib added inline comments.Aug 9 2023, 11:17 AM
lldb/include/lldb/Host/StreamFile.h
12

There is already a GetFileSP we could use that instead and remove GetFile

bulbazord added inline comments.Aug 9 2023, 11:20 AM
lldb/include/lldb/Host/StreamFile.h
12

Yeah, that would be a good idea. Especially since GetFile() itself doesn't check the validity of the FileSP it has anyway... And it doesn't seem like there's an obvious way to check for validity before calling GetFile(). Is it ok if I do that in a follow-up? I don't want a revert to move the file back to lldbCore in case something goes wrong.

mib accepted this revision.Aug 9 2023, 11:24 AM
mib added inline comments.
lldb/include/lldb/Host/StreamFile.h
12

Sure

This revision is now accepted and ready to land.Aug 9 2023, 11:24 AM
This revision was automatically updated to reflect the committed changes.