Remove unnecessary null pointer checks

[elfring]

An extra null pointer check is not needed in functions like the following.

• nnef_graph_release
• nnef_tensor_release

[gyenesvi]

Maybe not required, but does not mean any problem/performance hit either. On the other hand, it clarifies what actually happens (that the pointer might actually be null).

[elfring]

💭 Would you become interested to use a development tool like “clang-tidy” for corresponding source code adjustments?

[gyenesvi]

Not sure why I would; you did not even reflect to my comment. Things that work don't need fixing. And readability is a subjective matter.

[elfring]

💭 Would you like to take the C++ guideline “R.11: Avoid calling new and delete explicitly” better into account?

[gyenesvi]

As you may see, this code is intended to be used in a C environment, not C++, so the above reference is out of context. Raw pointer usage is unavoidable here.

On the other hand, your answers sound like that of a bot. Are you a bot?

[elfring]

…, this code is intended to be used in a C environment, …

👀 Some delete statements are obviously still used in your C++ source files.

Raw pointer usage is unavoidable here.

💭 Mentioned implementation details can be adjusted.

Are you a bot?

No.

[gyenesvi]

Given that raw pointers are unavoidable due to C environment, delete/free statement is also necessary, smart pointers cannot be used here, at least I cannot see another way.

[elfring]

…, smart pointers cannot be used here,

👀 I got an opposite impression for C++ source code which is generally improvable.

at least I cannot see another way.

💭 Your temporary view might be too limited.

[gyenesvi]

If you have a proposal for improving it, let me know (by code examples).

[elfring]

Known development tools can remind (interested programmers and reviewers) on possible code improvements already. 🤔

[gyenesvi]

But I want to know if you can propose an improvement. Whether you can participate in this discussion constructively and cooperatively. You haven't convinced me that you are not a bot.

[elfring]

But I want to know if you can propose an improvement.

I obviously did that for various software components.

💭 I hope that development interests will grow for reported change possibilities.

You haven't convinced me that you are not a bot.

How much does such an enquiry matter for this issue? 🤔

[gyenesvi]

It shows me why this discussion is not constructive, and does not lead anywhere. Maybe that is why you cannot propose an improvement relevant to this piece of code, with code examples, only point to generic rules and principles.

[elfring]

💭

• Do you care for advices from linked information sources at all?
• Would you dare to read them once more?

[raltnoeder]

If I may interject, @elfring if you're gonna rework code that isn't really broken, you might as well suggest more comprehensive improvements for a reworked/modernized release rather than suggesting a seemingly random isolated change that's probably one of the more uninteresting ones in the first place.

E.g., assuming at least C++11 and C99, you would probably change

void nnef_graph_release( nnef_graph_t graph )
{
    Graph *nnef_graph = (Graph *)graph;
    if ( nnef_graph )
    {
        delete nnef_graph;
    }
}

to

void nnef_graph_release(nnef_graph_t graph)
{
    delete static_cast<Graph*> (graph);
}

Anyway, I'm not sure why you are focusing on this specific if statement, while you seem to ignore a whole bunch of more interesting topics.
E.g., this function:

int nnef_tensor_read( const char* path, nnef_tensor_t tensor, char *perror )
{
  Tensor *nnef_tensor = (Tensor *)tensor;
  if ( nnef_tensor == NULL )
  {
    return 0;
  }

  std::string error;
  if ( !read_tensor(path, *nnef_tensor, error) )
  {
    if ( perror != NULL )
    {
      strncpy(perror, error.c_str(), error.length() + 1);
    }
    return 0;
  }
  return 1;
}

which could possibly be turned into:

bool nnef_tensor_red(const char* const path, nnef_tensor_t tensor, char* const error_out)
{
  bool success_flag = false;
  Tensor* const nnef_tensor = static_cast<Tensor*> tensor;
  if (nnef_tensor != nullptr)
  {
    // FIXME: Does it have std::bad_alloc handled somewhere?
    std::string error;
    success_flag = read_tensor(path, *nnef_tensor, error);
    if (!success_flag)
    {
      if (error_out != nullptr)
      {
        // FIXME: Potential buffer overflow?
        std::strncpy(error_out, error.c_str(), error.length() + 1);
      }
    }
  }
  return success_flag;
}

changing:

• bool instead of int, provided the C compilers are modern enough
• add some const qualifiers where they seem appropriate
• consistently use C++ style pointer format
• use C++ style casts, provided the C++ compilers are modern enough
• use C++ style nullptr, provided the C++ compilers are modern enough
• avoid shadowing the perror function
• we have a clean flow through the function with a single return statement at the end now instead of just jumping out from several places with arbitrary explicit return codes

More importantly though, the use of strncpy caught my attention, because it seems to have the potential for a buffer overflow, since the copy loop is limited to the length of the source buffer rather than the length of the destination buffer, but I did not dig any deeper, so you might want to double check this.

Also, the std::string constructor might throw if an allocation fails, and depending on whether read_tensor handles exceptions, it might throw too, as it seems to append error messages to the string, so whether failed allocations are handled properly should probably be double checked too.