search

named-data-iot / ndn-lite

A lightweight NDN protocol stack with high-level application support including security bootstrapping, access control, trust management, etc.
https://ndn-lite.named-data.net
GNU Lesser General Public License v3.0
44 stars 16 forks source link

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

Closed yan-foto closed 4 years ago

yan-foto commented 4 years ago

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

https://github.com/named-data-iot/ndn-lite/blob/510c4ec7960c79ea6029419698847cc8d8c5c9da/security/ndn-lite-hmac.c#L146-L150

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

tianyuan129 commented 4 years ago

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.

yoursunny commented 4 years ago

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?

yan-foto commented 4 years ago

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 ?

yoursunny commented 4 years ago

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.

yan-foto commented 4 years ago

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?

yoursunny commented 4 years ago

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.

yan-foto commented 4 years ago

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.

yan-foto commented 4 years ago

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...

yoursunny commented 4 years ago

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.

Zhiyi-Zhang commented 4 years ago

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!