I've never used clang before, so I thought I'd give it a spin to see what kind of errors it generates.

There are a number of the form:

char buff[10];

.....

if( NULL != buff) {

  do_something(buff);  // do it even if buff[0]=0!

}

For example:

ibnml/nml/nml_mod.cc:274:35: warning: comparison of array 'this->commandIn->cms->ProcessName' not equal to a null pointer is always true [-Wtautological-pointer-compare]
      if (NULL != commandIn->cms->ProcessName)
          ~~~~    ~~~~~~~~~~~~~~~~^~~~~~~~~~~

The fix is easy:

diff --git a/src/libnml/nml/nml_mod.cc b/src/libnml/nml/nml_mod.cc
index f0dd5f2b14..0b5059e032 100644
--- a/src/libnml/nml/nml_mod.cc
+++ b/src/libnml/nml/nml_mod.cc
@@ -163,6 +163,7 @@ NML_MODULE::zero_common_vars ()
   min_run_time = 1e6;
   max_run_time = 0;
   start_cycle_time = 0;
+  proc_name = 0;
   temp_file = 0;
   temp_line = 0;
   Dclock_expiration = 0;
@@ -271,7 +272,7 @@ NML_MODULE::setCmdChannel (RCS_CMD_CHANNEL * cmd_channel)
     }
   if (NULL != commandIn->cms)
     {
-      if (NULL != commandIn->cms->ProcessName)
+      if (*commandIn->cms->ProcessName)
        {
          proc_name =
            (char *) malloc (strlen (commandIn->cms->ProcessName) + 1);

Changes of this type will result in code paths previously traversed not being taken when the buffers are empty. In the above example proc_name was never initialized in the constructor; not sure what impact that had.

This brings up a few questions:

What is the preferred way for submitting these? Is it better to make a bunch small pull requests / patches? For example, all affected files in a single directory?  Or multiple files across multiple directories, but with similar errors.

From reading the developer docs, it seems I should make a branch on github and have buildbot build them. I'm not sure I understand the mechanics of  that however.

Should I first raise this these as a github issue?

I ran the test suit. It didn't return any errors.

Regarding the test suite, it looks like its mostly (all?) functional test. Would it be worth having unit tests for some of the code? I've used google test a fair bit, combining that with Valgrind to find memory leaks. Full blown unit testing would be a huge undertaking. A more realistic approach would be to focus an code being fixed and related functions.

Thanks.




_______________________________________________
Emc-developers mailing list
Emc-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/emc-developers

Reply via email to