Skip to content
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

coap minimal #212

Merged
merged 1 commit into from Mar 9, 2015
Merged

coap minimal #212

merged 1 commit into from Mar 9, 2015

Conversation

alashkin
Copy link
Contributor

@alashkin alashkin commented Mar 9, 2015

Hi, @mmikulicic !

Please, review this PR.
Since this is my first protocol added to fossa, I'm not sure that I'm right in details.

Please, take a look. (aka PTAL)

@mkmik
Copy link
Collaborator

mkmik commented Mar 9, 2015

FYI: PTAL means Please Take Another Look

@alashkin
Copy link
Contributor Author

alashkin commented Mar 9, 2015

FYI: PTAL means Please Take Another Look

BTW, Urban dictionary offers another :-)

* FOSSA functions
*/

uint32_t ns_coap_send_message(struct ns_connection *nc,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a documentation extraction from comments.

It works if a comment is right before a function declaration.

I noticed that:

/* Options memory management functions */
void ns_coap_free_options(struct ns_coap_message *cm) {

Is meant to be a grouping comment but ended up being interpreted as a function comment.

Why not just add a short comment for all exported functions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not just add a short comment for all exported functions ?

Why not? I'll add. :)

@mkmik
Copy link
Collaborator

mkmik commented Mar 9, 2015

LGTM, but why don't you develop in our main repo so that we get circleci tests ?

@alashkin
Copy link
Contributor Author

alashkin commented Mar 9, 2015

but why don't you develop in our main repo so that we get circleci tests ?

I started this PR in my fork, so I decided to finish it there.
But, next PR about coap will be from main repo's branch.

@mkmik
Copy link
Collaborator

mkmik commented Mar 9, 2015

Ok, you can run the same tests as the CI locally with:

make -C test docker

The unit test fails in C++ mode:

docker run --rm -v /Users/mkm/Projects/cesanta/fossa/test/../..:/cesanta cesanta/fossa_test
make: Entering directory `/cesanta/fossa/test'
CLEAN   coverage
CLEAN   all
make[1]: Entering directory `/cesanta/fossa/test'
CC      unit_test_cxx
CC      unit_test_ansi
CC      unit_test_c99
CC      unit_test_c11
unit_test.c: In function 'const char* test_coap()':
unit_test.c:2107:22: error: narrowing conversion of '233' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
     0xd1, 0x23, 0x11 };
                      ^
unit_test.c:2107:22: error: narrowing conversion of '144' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
unit_test.c:2107:22: error: narrowing conversion of '184' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
unit_test.c:2107:22: error: narrowing conversion of '209' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
unit_test.c:2109:28: error: narrowing conversion of '233' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
     0x60, 0x00, 0xe9, 0x1b };
                            ^
unit_test.c:2127:10: error: narrowing conversion of '144' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
     0x2e };
          ^
unit_test.c:2127:10: error: narrowing conversion of '255' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
unit_test.c:2134:40: error: narrowing conversion of '149' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
     0x6d, 0x79, 0x64, 0x61, 0x74, 0x61 };
                                        ^
unit_test.c:2134:40: error: narrowing conversion of '183' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
unit_test.c:2134:40: error: narrowing conversion of '255' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
unit_test.c:2136:28: error: narrowing conversion of '255' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
     0xFF, 0x00, 0xFF, 0x00 };
                            ^
unit_test.c:2136:28: error: narrowing conversion of '255' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
unit_test.c:2141:40: error: narrowing conversion of '149' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
     0x6d, 0x79, 0x64, 0x61, 0x74, 0x61 };
                                        ^
unit_test.c:2141:40: error: narrowing conversion of '183' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
unit_test.c:2141:40: error: narrowing conversion of '241' from 'int' to 'char' inside { } is ill-formed in C++11 [-Werror=narrowing]
cc1plus: all warnings being treated as errors
make[1]: *** [unit_test-cxx] Error 1
make[1]: Leaving directory `/cesanta/fossa/test'
make: *** [compile] Error 2
make: Leaving directory `/cesanta/fossa/test'
make: *** [docker] Error 2

@mkmik
Copy link
Collaborator

mkmik commented Mar 9, 2015

Another minor thing. Please follow our styleguide for git commit messages (refer to github styleguide)

make setup-hooks will check a few things about commit message styles.

@alashkin
Copy link
Contributor Author

alashkin commented Mar 9, 2015

Marko,

  1. Comments fixed.
  2. Unit-tests fixed.
  3. Commit message updated.

And finally - I promise, tomorrow I'll move to main repo. No more independent PR!

PTAL

@mkmik
Copy link
Collaborator

mkmik commented Mar 9, 2015

Looks good! Merging

mkmik pushed a commit that referenced this pull request Mar 9, 2015
@mkmik mkmik merged commit c2a0d58 into cesanta:master Mar 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants