Skip to content

Yaml param parser shane review 2 - #241

Merged
anup-pem merged 2 commits into
yaml_param_parserfrom
yaml_param_parser_shane_review_2
May 25, 2018
Merged

Yaml param parser shane review 2#241
anup-pem merged 2 commits into
yaml_param_parserfrom
yaml_param_parser_shane_review_2

Conversation

@sloretz

@sloretz sloretz commented May 25, 2018

Copy link
Copy Markdown
Contributor

A couple changes for windows:

1: Remove include uinistd.h
2: Use size_t for array indexes to eliminate warnings about possible loss of data

@sloretz sloretz added the in progress Actively being worked on (Kanban column) label May 25, 2018

@mikaelarguedas mikaelarguedas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, That's the branch I'v been using for testing on windows 👍.

Just checking: Did you run CI with that branch on the other platforms ?

@sloretz

sloretz commented May 25, 2018

Copy link
Copy Markdown
Contributor Author

@mikaelarguedas I have not. I know it at least works in my xenial docker container locally, but have not tested OSX or the arm job.

@mikaelarguedas

Copy link
Copy Markdown
Member

Can you please run a MacOS job? (linux is not necessary if it works on your machine)

@sloretz

sloretz commented May 25, 2018

Copy link
Copy Markdown
Contributor Author

@mikaelarguedas OSX with this branch and libyaml_vendor Build Status

@mikaelarguedas

Copy link
Copy Markdown
Member

Thanks 👍

@anup-pem

Copy link
Copy Markdown
Contributor

Looks good. Will go ahead and merge it.

@anup-pem
anup-pem merged commit b4c2161 into yaml_param_parser May 25, 2018
@sloretz
sloretz deleted the yaml_param_parser_shane_review_2 branch May 25, 2018 18:08
@sloretz sloretz mentioned this pull request May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in progress Actively being worked on (Kanban column)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants