Fix compiler warnings

[yan-foto]

As discussed in #74 here is a set of changes that fix compiler warnings. Some of the changes are trivial and non-critical while the others fix some hidden bugs such as missing return statements and possibly unsafe pointer arithmetic.

Each commit addresses a specific warning type and additional information (if applicable) is also included in commit message.

[yan-foto]

I still get a number of unused-parameter warnings which I have postponed for the future. Maybe you guys can take a look at them.

[yoursunny]

It’s necessary to have continuous integration that turns on -Werror to ensure these warnings do not reappear. Warnings that have not been fixed can then be turned off via -Wno-error=unused-parameter.

[yoursunny]

I tested f0998b425247cf13af2f870c6d614e6b6d1a6476 on Ubuntu 16.04 using GCC 5.4.0.

I'm still seeing warnings (that do not belong in test files). For example:

/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘_on_ac_notification’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:141:22: warning: passing argument 1 of ‘ndn_name_print’ from incompatible pointer type [-Wincompatible-pointer-types]
   NDN_LOG_DEBUG_NAME(&notification);
                      ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:41:18: note: in definition of macro ‘NDN_LOG_DEBUG_NAME’
   ndn_name_print(name);\
                  ^
In file included from /tmp/ndn-lite/tests/ndn-lite/app-support/../encode/interest.h:14:0,
                 from /tmp/ndn-lite/tests/ndn-lite/app-support/access-control.h:14,
                 from /tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:11:
/tmp/ndn-lite/tests/ndn-lite/app-support/../encode/name.h:42:1: note: expected ‘const ndn_name_t * {aka const struct ndn_name *}’ but argument is of type ‘ndn_interest_t * {aka struct ndn_interest *}’
 ndn_name_print(const ndn_name_t* name);
 ^
In file included from /tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:30:0:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:149:21: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘uint32_t {aka unsigned int}’ [-Wformat=]
       NDN_LOG_DEBUG("[ACCESSCTL] Enforced update for Service %u, KeyID %lu\n",
                     ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘_on_ekey_data’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:208:17: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘uint32_t {aka unsigned int}’ [-Wformat=]
   NDN_LOG_DEBUG("[ACCESSCTL] AES KeyID = %lu \n", keyid);
                 ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:253:17: warning: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 2 has type ‘ndn_time_us_t {aka long unsigned int}’ [-Wformat=]
   NDN_LOG_DEBUG("[ACCESSCTL] Key update: %lluus\n", m_measure_tp2 - m_measure_t
                 ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘_on_dkey_data’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:308:17: warning: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘uint32_t {aka unsigned int}’ [-Wformat=]
   NDN_LOG_DEBUG("[ACCESSCTL] AES KeyID = %lu \n", keyid);
                 ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘_express_ekey_interest’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:394:35: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
   ndn_name_t* self_identity_key = ndn_key_storage_get_self_identity_key(service
                                   ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:399:60: warning: passing argument 3 of ‘ndn_signed_interest_ecdsa_sign’ from incompatible pointer type [-Wincompatible-pointer-types]
   ndn_signed_interest_ecdsa_sign(&interest, self_identity, self_identity_key);
                                                            ^
In file included from /tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:15:0:
/tmp/ndn-lite/tests/ndn-lite/app-support/../encode/signed-interest.h:43:1: note: expected ‘const ndn_ecc_prv_t * {aka const struct ndn_ecc_prv *}’ but argument is of type ‘ndn_name_t * {aka struct ndn_name *}’
 ndn_signed_interest_ecdsa_sign(ndn_interest_t* interest,
 ^
In file included from /tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:30:0:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘ndn_ac_trigger_expiration’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:577:19: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 2 has type ‘uint32_t {aka unsigned int}’ [-Wformat=]
     NDN_LOG_DEBUG("[ACCESSCTL] Local Decryption Key %ld forced expired\n", aes_
                   ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:581:19: warning: format ‘%ld’ expects argument of type ‘long int’, but argument 2 has type ‘uint32_t {aka unsigned int}’ [-Wformat=]
     NDN_LOG_DEBUG("[ACCESSCTL] Notifying Encryption Key %ld forced expired\n",
                   ^
/tmp/ndn-lite/tests/ndn-lite/app-support/../util/logger.h:38:10: note: in definition of macro ‘NDN_LOG_DEBUG’
   printf(__VA_ARGS__); \
          ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘_on_ac_notification’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:158:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c: In function ‘ndn_ac_trigger_expiration’:
/tmp/ndn-lite/tests/ndn-lite/app-support/access-control.c:599:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^

The proper way to resolve -Wformat is to #include <inttypes.h> and then use PRIu64 etc. For things like ndn_time_us_t, add a #define PRI_ndn_time_us_t PRIu64 right next to the typedef declaring that type, then use PRI_ndn_time_us_t macro when you need to print a value of that type.

[yan-foto]

The proper way to resolve -Wformat is to #include and then use PRIu64 etc. For things like ndn_time_us_t, add a #define PRI_ndn_time_us_t PRIu64 right next to the typedef declaring that time, then use PRI_ndn_time_us_t macro when you need to print a value of that type.

Yes, that also came to my attention after I posted the comment. This is obviously due to my lack of experience in writing portable/cross platform code!Thanks for the hint! I'll take care of it tomorrow. 

[yan-foto]

The latest changes address warnings related to adaptation source code (ignored in original PR commits). I also updated CMake files so make would fail on warnings (only for NDN lite source and not tests).

NOTE: the code cannot be properly built in this state. There are yet some compiler warnings that I'm not sure how to address and need attention of original authors.

[yoursunny]

There are yet some compiler warnings that I'm not sure how to address and need attention of original authors.

Use git blame to find out the author of each function, and @ them to participate in this review.

[yan-foto]

Use git blame to find out the author of each function, and @ them to participate in this review.

I feel as if I'm being promoted to project manager for NDN lite!

Anyhow, @Zhiyi-Zhang and @tianyuan129 seem to be the only developers involved in this function of access-control.c. Note that the warnings does not end here. So you guys need to decide how to handle or ignore them (by adding proper Wno-* flags to CMake file).

[yoursunny]

So you guys need to decide how to handle or ignore them (by adding proper Wno-* flags).

It is inappropriate to ignore -Wreturn-type, because the caller would read garbage value.

However, for _on_ac_notification, this appears to be an internal function and not part of public API. Then, you can add a return value expected by its callsite (see ndn_on_interest_func definition), and add static to the function declaration to ensure that it remains an internal function that cannot be used elsewhere.

Note that the warnings does not end here.

You can list each of the remaining warnings, post them here as comments and @ the original author to figure out a solution.

[tianyuan129]

However, for _on_ac_notification, this appears to be an internal function and not part of public API.

Yes, this is an internal function and not part of public API. The return value should be NDN_SUCCESS, and static should be added to the function declaration.

[yan-foto]

It is inappropriate to ignore -Wreturn-type, because the caller would read garbage value.

I meant in general, not in this specific case.

You can list each of the remaining warnings, post them here as comments and @ the original author to figure out a solution.

My time is really tight here. I'd appreciate it if the last mile can be taken care of by the core team.

Yes, this is an internal function and not part of public API. The return value should be NDN_SUCCESS, and static should be added to the function declaration.

I can change this quickly, but could you take a look at the rest?

[yan-foto]

My time is really tight here. I'd appreciate it if the last mile can be taken care of by the core team.

@yoursunny this is exactly what I'm talking about 👆

[yoursunny]

I'd appreciate it if the last mile can be taken care of by the core team.

I think you are a better developer than most others, and you deserve a spot on the core team or become the project manager.

[tianyuan129]

I can change this quickly, but could you take a look at the rest?

sure, can I merge it first your project can move on?

[yoursunny]

sure, can I merge it first your project can move on?

I'd recommend not to merge the current code, since it leaves the codebase in an uncompilable state. All warnings have to be resolved.

[yan-foto]

I think you are a better developer than most others, and you deserve a spot on the core team or become the project manager.

you should've been a politician or at least be awarded with a medal or something for your convincing skills :)

sure, can I merge it first your project can move on?

Thanks for the offer, but let's wait until everything is resolved. @yoursunny somehow convinced me to do take care of the rest...

[yan-foto]

So the warnings are fixed and the tests all pass. During this process I also discovered some bugs which makes me really wonder why these were not flagged by the unit tests :thinking:

Anyhow, it has been a while since I've used Travis CI on GitHub and according to [its docs] you should first enable it for GitHub before using it. If I understand it correctly, it doesn't pick up jobs automatically anymore as it used to do. Nonetheless I added a preliminary CI script, wondering if it's gonna work!

[yoursunny]

you should first enable it for GitHub before using it

For Travis CI, you need to enable it on your fork first. Independently, a maintainer needs to enable it for this repository.

[yan-foto]

OK folks, everything works as expected. I really didn't want to work on shabbat, but I guess I also couldn't leave this cake half-baked!

The unit tests pass but I still think the code needs more rigorous testing. So maybe we first merge this on the develop branch and have it tested by some of our best (students). What do you reckon?

[yoursunny]

@tianyuan129 needs to install Travis CI in this repository.

[tianyuan129]

Sure. Currently I'm not the admin for this repository. Let me ask for the permission first and then I'll install it.

[yoursunny]

@Zhiyi-Zhang needs to install Travis CI in this repository.

[tianyuan129]

just installed Travis CI on develop branch: d60a114.