Skip to content

Conversation

cgundogan
Copy link
Member

@cgundogan cgundogan commented Apr 1, 2020

Contribution description

Since we also allow the use of non-\0-terminated strings, we have to remove this check here. Otherwise, it returns -1 for those strings, although parsing was successful.
The check was overcautious anyway.

I changed the diff now to check for uri_end instead of completely removing it.

Testing procedure

unittests should still run

Issues/PRs references

none

@cgundogan cgundogan added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Area: sys Area: System labels Apr 1, 2020
@cgundogan cgundogan requested a review from miri64 April 1, 2020 23:42
@cgundogan cgundogan force-pushed the pr/uri_parser/remove_check_for_zero branch from c5f0b42 to 9b2f7bc Compare April 2, 2020 08:05
@cgundogan
Copy link
Member Author

added a test that fails on master and works with the patch

@cgundogan cgundogan changed the title sys/uri_parser: remove check for zero sys/uri_parser: check for uri_end instead of 0 Apr 2, 2020
@cgundogan cgundogan force-pushed the pr/uri_parser/remove_check_for_zero branch from 9b2f7bc to 1bfc1fb Compare April 2, 2020 13:11
@cgundogan
Copy link
Member Author

@miri64 addressed your comments and amended immediately. Looks much nicer with VEC_CHECK()

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Apr 3, 2020
@cgundogan cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 3, 2020
@cgundogan
Copy link
Member Author

retriggered murdock because of a test timeout

@cgundogan
Copy link
Member Author

murdock agrees now

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK! In addition to the tests running on the CI, I ran tests-uri_parse alone on a samr21-xpro. They pass.

@miri64 miri64 merged commit d00bde7 into RIOT-OS:master Apr 3, 2020
@cgundogan cgundogan deleted the pr/uri_parser/remove_check_for_zero branch April 3, 2020 11:21
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants