-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Modify reflection dumping to use std::ostream instead of FILE * #40452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@swift-ci smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My assumption is that (up until now) the only client of this API is swift-reflection-dump, so this looks like a good change to make the API more useful for LLDB.
void MetadataSource::dump() const { | ||
dump(stderr, 0); | ||
} | ||
void MetadataSource::dump() const { dump(std::cout, 0); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, and various others, should be cerr
to match the previous code, although I'm not sure offhand why we use one versus the other.
auto oldFlags = stream.flags(); | ||
stream << std::fixed << std::setprecision((int)fieldName.size()) | ||
<< fieldName.data(); | ||
stream.flags(oldFlags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++ really makes this difficult, huh. I think stream.write()
might be better here, even though it doesn't quite mean the same thing (since it won't stop at a NUL character).
@@ -200,7 +201,7 @@ swift_reflection_addReflectionInfo(SwiftReflectionContextRef ContextRef, | |||
|| Info.capture.offset != 0 | |||
|| Info.type_references.offset != 0 | |||
|| Info.reflection_strings.offset != 0) { | |||
fprintf(stderr, "reserved field in swift_reflection_info_t is not zero\n"); | |||
std::cout << "reserved field in swift_reflection_info_t is not zero\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one should definitely be cerr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a future patch we should upgrade this to do real error handling. Xcode users would not see anything printed to stderr when it happens in lldb-rpc-server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of an assertion failure than an error, but better handling wouldn't be a bad thing.
cbada90
to
385d91a
Compare
@swift-ci test |
@swift-ci smoke test macOS |
This change is necessary so lldb can use typeref dumping facilities directly.