-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Macros] Update plugin search options serialization #66689
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
[Macros] Update plugin search options serialization #66689
Conversation
Previously plugin search options were serialized for each option kind. Instead serialize them in the order specified.
swiftlang/llvm-project#7003 |
options_block::ResilienceStrategyLayout::readRecord(scratch, optKind); | ||
switch (PluginSearchOptionKind(optKind)) { | ||
case PluginSearchOptionKind::PluginPath: | ||
optStr = "-plugin-path"; |
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.
Not sure about this. But I didn't want to introduce another "kind" enum just for swift/Serialization/Validation.h
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.
The fact that LLDB needs to match on these strings feels a little brittle. Would we be better off exporting PluginSearchOptionKind
into a header and having that in the list of plugin search options?
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.
👍 Added a commit.
Made PluginSearchOption
a struct with PluginSearchOption::Kind
in SearchPathOptions.h
options_block::ResilienceStrategyLayout::readRecord(scratch, optKind); | ||
switch (PluginSearchOptionKind(optKind)) { | ||
case PluginSearchOptionKind::PluginPath: | ||
optStr = "-plugin-path"; |
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.
The fact that LLDB needs to match on these strings feels a little brittle. Would we be better off exporting PluginSearchOptionKind
into a header and having that in the list of plugin search options?
LoadPluginLibrary, | ||
LoadPluginExecutable, | ||
}; | ||
using PluginSearchOptionKindField = BCFixed<3>; |
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.
It looks like a BCFixed<2>
should be enough for the current enum size.
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.
The change looks good!
Create a 'Kind' enum so that deserialization can use the kind instead of a string option name.
ec184e9
to
6fa0c14
Compare
swiftlang/llvm-project#7003 |
1 similar comment
swiftlang/llvm-project#7003 |
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.
LGTM other than @xymus comment
|
Previously plugin search options were serialized for each option kind. Instead serialize them in the order specified.
rdar://110903149