v1 fix #1588: передача пропущенных параметров в COM-объекты#1589
v1 fix #1588: передача пропущенных параметров в COM-объекты#1589EvilBeaver merged 3 commits intoEvilBeaver:latestfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
Fixes handling of missing parameters when calling COM objects by distinguishing between undefined values and truly missing parameters. The change ensures COM objects receive proper Missing.Value for omitted parameters instead of null values.
- Modified PrepareDynamicArgs to accept context and skip processing for COM objects
- Updated ConvertToCLRObject to handle NotAValidValue type and return Missing.Value
- Simplified COM marshalling logic by removing redundant null coalescing
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| MachineInstance.cs | Added context parameter to PrepareDynamicArgs and special handling for COM objects |
| ContextValuesMarshaller.cs | Added NotAValidValue case returning Missing.Value and simplified object conversion logic |
| COMWrapperContext.cs | Removed redundant null coalescing in MarshalIValue method |
| if (val.GetRawValue() is IObjectWrapper wrapped) | ||
| result = wrapped.UnderlyingObject; | ||
| else | ||
| throw ValueMarshallingException.NoConversionToCLR(val.GetType()); | ||
|
|
||
| return result; | ||
| return wrapped.UnderlyingObject; | ||
|
|
There was a problem hiding this comment.
The code assumes that if val.DataType is not one of the handled primitive types and val.GetRawValue() is not IObjectWrapper, then it should throw an exception. However, the original code had a specific check for DataType.Object before checking for IObjectWrapper. This change may cause exceptions for valid Object types that are not IObjectWrapper instances.
There was a problem hiding this comment.
В исходном коде результат присваивания result = val.AsObject(); после проверки val.DataType == DataType.Object либо перезаписывался, либо терялся при исключении.
Исправленный код не меняет это поведение, но возможно, логика преобразования была неверной изначально.
There was a problem hiding this comment.
Ах блин, ты же тупой, не умеешь отвечать на комменты по ревью.
There was a problem hiding this comment.
@coderabbitai оцени изначальный комментарий от @copilot на правильность и ответ на него от Mr-Rm
There was a problem hiding this comment.
@Mr-Rm @EvilBeaver мои полномочия здесь всё :D
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Судя по использованию ConvertToCLRObject внутри движка, нужно добавить обработку DataType.Object. Но раньше влияние этой ошибки вылезало ли где-нибудь?
А вот что будет с передачей полученного object'а по COM, судить не возьмусь. Впрочем, wrapped так бы и передался как отмаршаленный CLR-object. Некоторые подробности есть здесь
Предложение: пофиксить DataType.Object, а COM-серверы пусть сами разбираются с тем, что пришло.
There was a problem hiding this comment.
раньше влияние этой ошибки вылезало ли где-нибудь
нет, не сталкивался
Предложение: пофиксить DataType.Object, а COM-серверы пусть сами разбираются с тем, что пришло.
Я вообще мало что понял в ошибке, уверен ты знаешь лучше меня
|
@Mr-Rm прости за задержку, я довольно надолго выпал. Что с этим PR? Его вливать, не вливать, надо разрешить какие-то вопросы? |
|
(Прошу прощения, я сам отключался и пока непонятно, надолго ли) |
Остается вопрос, какие еще динамические объекты отличают передачу
Неопределеноот пропущенного параметра. Возможно, потребуется модификация PrepareDynamicArgs()