-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
YAML : Constraints with nullable types fixed for Min, Max & NotValue #12958
YAML : Constraints with nullable types fixed for Min, Max & NotValue #12958
Conversation
PR #12958: Size comparison from ed367ec to 384dc3f Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
384dc3f
to
233fb01
Compare
PR #12958: Size comparison from c9bc5ae to 233fb01 Increases above 0.2%:
Increases (1 build for linux)
Full report (30 builds for efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
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.
Thank you, that was a lot more tests than I was expecting.... ;)
As the CI shows, this does in fact need darwin changes. You'll want to at least run ./scripts/tools/zap/generate.py -t src/darwin/Framework/CHIP/templates/templates.json src/controller/data_model/controller-clusters.zap
to regen there. After that, look at the generated code; I expect the codegen in src/darwin/Framework/CHIP/templates/partials/test_cluster.zapt
will probably need to be adjusted to handle Nullable by using a helper method that checks for null before calling XCTAssert* functions....
Ok i ll look into this , never did objective-c guess it is time to learn |
So what will happen in the Objc code is that So just wrapping a |
03d6446
to
2307ed4
Compare
2307ed4
to
bbea0a6
Compare
PR #12958: Size comparison from 8bedafa to bbea0a6 Increases above 0.2%:
Increases (1 build for linux)
Full report (20 builds for efr32, k32w, linux, mbed, p6, qpg, telink)
|
PR #12958: Size comparison from 8bedafa to 868f8f0 Increases above 0.2%:
Increases (1 build for linux)
Decreases (1 build for esp32)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
PR #12958: Size comparison from 8bedafa to bfdb108 Increases above 0.2%:
Increases (26 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (3 builds for mbed, p6)
Full report (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
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.
Approving, but fixing the darwin code to use asTypedLiteral would have fixed those 64-bit things.... I'll do that as a followup.
|
Problem
What is being fixed? Examples:
Change overview
Modification to support nullable types
This PR is needed to lay down other PR and new YAMLs Tests
EDITs: We allow NULL values to PASS here, we check MIN MAX only if the value is present
since we don't want to fail on such condition, there is another tag dedicated to check the nullity in particular.
We just allow for a test to verify the value is withing a particular range and a nullable attribute is allowed to be NULL
Testing
How was this tested? (at least one bullet point required)