dusty-nv / jetson-utils

C++/CUDA/Python multimedia utilities for NVIDIA Jetson
MIT License
737 stars 289 forks source link

Not using braces and the logging library is error prone #183

Open andrejlevkovitch opened 12 months ago

andrejlevkovitch commented 12 months ago

In the repository, for some reason, braces are not using in case when there is just one line after if or else. This is already a potential problem, because that is extremely error prone approach, especially if formatter (like clang-format) is not in use. For demonstrate how bad it is I want to show you how compiler handles one of if-else statements in the code.

Here it is: https://github.com/dusty-nv/jetson-utils/blob/d374d425e332f5732204407fdaba8cc271e376ea/video/videoOptions.cpp#L55-L58

So, according to the lines, I expect that if prefix is NULL I should see video options:\n line in the logs. But here is a problem: the if-else doesn't have braces and it uses macros-es inside. Macros is not a function, so a priori you should not make assumptions about how much lines it takes. One line macros is not always equal to just line of code for compiler. And in that particular case it leads to a bug (not fatal, but still). Let's expand that macros-es:

if (prefix != NULL)
  GenericLogMessage(Log::Info, LOG_LEVEL_PREFIX_INFO "%s video options:\n", prefix);
else
  GenericLogMessage(Log::Info, LOG_LEVEL_PREFIX_INFO "video options:\n");

GenericLogMessage is also a macros, so lets expand further:

if (prefix != NULL)
  if (Log::Info <= Log::GetLevel()) fprintf(Log::GetFile(), LOG_LEVEL_PREFIX_INFO "%s video options:\n", prefix);
else
  if (Log::Info <= Log::GetLevel()) fprintf(Log::GetFile(), LOG_LEVEL_PREFIX_INFO "video options:\n");

Hm... So, here is a question: to what if that else relates?

PS I hope that you will abandoned that error prone style and start using braces even if there is "just one line of code". There are some tools that can help with that migration, like clang-format and clang-tidy

dusty-nv commented 12 months ago

Hi @andrejlevkovitch, thanks for raising this and appreciate your PR's - just merged them.