-
Notifications
You must be signed in to change notification settings - Fork 56
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
Temporarily disable test that check const fields #94
Temporarily disable test that check const fields #94
Conversation
(we should release 2.6.6 with this ASAP to remove irrelevant warning spam in 2.6.5) |
@@ -734,10 +734,10 @@ unittest | |||
{ | |||
static struct CS | |||
{ | |||
cstring s; | |||
mstring s; |
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.
Will this change need to be reverted in v4.x.x as well?
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.
No, this will be a breaking change in v4.x.x
- such struct will no longer deserialize (it was never intended to). I wonder though how will it affect bidding apps as I presume this test was added by @mathias-lang-sociomantic originally.
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.
Ok, we talked with @mathias-lang-sociomantic, looks like it was needed for one of work-in-progress ports and doesn't affect any merged code. We'll see if that code can be adjusted in a fashion simlar to swarm, for now this adjustments should be OK to merge.
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.
Is this change unrelated to "Temporarily disable test that check const fields" then?
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.
A bit indirectly. I will split it into separate commit and explain there.
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.
Done
"Serializer should reject a struct with 'istring'"); | ||
static assert(!is(typeof({Deserializer.deserialize!(II)(buffer2);})), | ||
"Deserializer should reject a struct with 'immutable' element"); | ||
version (none) |
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.
We should create an issue in v4.0.0 to reenable these tests.
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.
Because of deficiency in DMD implementation, combination of `pragma(msg)` and `is(typeof())` will result in message printed even if instantiation is speculative. As it is not possible to turn them into asserts yet, all tests that trigger warning are disabled for now.
It slipped through review by an accident some time ago - such use case was not intended and is going to be completely prohibited in ocean v4.0.0 Instead applications are supposed to use mutable structs for deserialization and apply head `const` if needed on serialized version.
c6f9f96
to
c905226
Compare
LGTM. |
Because of deficiency in DMD implementation, combination of
pragma(msg)
andis(typeof())
will result in message printed even if instantiation isspeculative. As it is not possible to turn them into asserts yet, all tests that
trigger warning are disabled for now.