named-data-iot/ndn-lite

Compiler warnings (sign-compare, type-limites, etc.)

Closed this issue · 10 comments

I have been porting ndn-lite to RIOT using the glue code from ndn-lite-over-riot. Compiling ndn-lite, however, throws a number of avoidable compiler errors (see bellow for examples) which can easily be fixed.

I just wanted to ask if there is anything that I need to take care of before submitting a PR?


Example

if (input_size < 0 || seed_size < 0
|| output_size > NDN_SEC_HMAC_MAX_OUTPUT_SIZE
|| info_size < 0) {
return NDN_INVALID_ARG;
}

Here are both inpute_size and seed_size unsigned so the comparison doesn't make sense (Wtype-limits)

Thanks for your interest. The library is written in standard C and requires a minimum version of C11, so it would be great if your changes are compatible with this requirement.

The library is written in standard C and requires a minimum version of C11, so it would be great if your changes are compatible with this requirement.

Can you add Continuous Integration scripts in this repository, to ensure this invariant does not get broken accidentally?

Thanks for quick responses. I'll get to it ASAP.

Thanks for your interest. The library is written in standard C and requires a minimum version of C11, so it would be great if your changes are compatible with this requirement.

Will do!

Can you add Continuous Integration scripts in this repository, to ensure this invariant does not get broken accidentally?

should I or do you mean @tianyuan129 ?

Can you add Continuous Integration scripts in this repository, to ensure this invariant does not get broken accidentally?

should I or do you mean @tianyuan129 ?

It doesn’t matter who does it, but fixing warnings without CI is not useful because the warnings would reappear shortly after with the sloppy code committed into this repository.

Just a quick reminder: some of these warnings are actual bugs and cannot be discarded as simple aesthetic fixes.

Do you guys have tests for this repository or do you just want to build it over the CI to make sure that the warnings are gone?

The tests are in https://github.com/named-data-iot/ndn-lite-tests repository, which also lacks CI. I told maintainer multiple times that tests have to be in the same repository, but they won’t listen.

If you cannot convince them, the chances are that neither can I! Nonetheless, if everyone (@yoursunny , @tianyuan129 , and @hwzh4640) consent, I will extend this PR to include tests (copy/paste) in this repository.

So I just integrated the tests including the git history for ndn-lite-tests for future reference. I used a cheap trick to avoid changes to the source code of tests (see last commit in #75) that might need some improvement/reconsideration.

Enabling CI is something that you guys as repository owners needs taking care of, as I don't have enough privileges to do so. If you're kind enough to merge this quickly, I can also finish up my work on RIOT's NDN package.

BTW the unit tests also produce a number of their own warnings...

Enabling CI is something that you guys as repository owners needs taking care of, as I don't have enough privileges to do so.

You can add GitHub Actions workflow YAML file as part of the Pull Request, and they should build automatically.

The tests are in https://github.com/named-data-iot/ndn-lite-tests repository, which also lacks CI. I told maintainer multiple times that tests have to be in the same repository, but they won’t listen.

Sorry for that. Currently most contributors of NDN-LITE are not available (me and Xinyu are at intern, Edward has graduated, GuanYu has finished his visiting at our lab) except Tianyuan, so we are really short of hands. It would be GREAT if @yoursunny and @yan-foto would like to contribute to this project.
I just saw @yan-foto's pull request. Thank you for your great work!