Remove redundant null terminate

[FabrizioSandri]

Originating Project/Creator
Affected Component
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)
Documentation Included (y/n)

---

Change Description

From the snprintf function documentation :

The functions snprintf() and vsnprintf() write at most size bytes (in‐cluding the terminating null byte ('\0')) to str.

So there's no need to null terminate the buffer.

Rationale

To avoid redundancy and improve overall readability of the code.

Testing/Review Recommendations

I made some changes in the component base classes. Specifically, I removed some redundant code that is already implemented in the snprintf function.

[timcanham]

This one is interesting... We may have confused it with strncpy.

[LeStarch]

I think the string may still not be terminated with a null in two cases:

1. Negative return code indicating error
2. Size of zero supplied.

I'd argue we should do something like:

FW_ASSERT(size > 0);
if (snprintf(buffer, size, "ActComp: %s", this->m_objName) < 0) {
    buffer[0] = 0;
}

This would ensure size is correct and it would set the buffer to an empty string in the case of an error.

[timcanham]

Is size == 0 always an error? Should we check for that and null terminate but not assert?

[FabrizioSandri]

If size == 0 it means that char* buffer variable is pointing to something not allocated. So we shouldn't null terminate the buffer anyway.

FW_ASSERT(size > 0); is correct

[LeStarch]

@timcanham, strictly speaking, you are right. One can supply a size-0 string to snprintf and it will not take action. However, in this functions case, we intend to print something out, so we'd expect the size to be >0 just as we expect the buffer to be defined.

@FabrizioSandri would you like to add those additional if checks or shall I?

[FabrizioSandri]

I just added the if checks.

[LeStarch]

I like these changes. They ensure safe use of snprintf

[ThibFrgsGmz]

Shouldn't the return value of snprintf be stored in a local variable that is then used in the if statement?

[FabrizioSandri]

@ThibFrgsGmz I think there's no need to store the result of the snprintf in a variable since it's used once in the if condition.

[LeStarch]

@ThibFrgsGmz can you elaborate? Since the return value is only used for the if check, this code should work. Is there a concern her I am not seeing?

[LeStarch]

I am going to go ahead and merge this in. If further concerns are raised, we can refactor.

[ThibFrgsGmz]

@LeStarch The line of code itself does not present any risk when it is executed. I just want to stress the importance of good practices in terms of readability and maintainability to avoid technical debt.

Typically, in my company, putting a function in a statement is strictly forbidden, except in exceptional cases like while(read(...)). In my experience and in discussions with colleagues who have worked in the railway, medical, automotive and aerospace industries (i.e. critical applications) this rule is also mandatory.

We believe that readability is more important nowadays, and that this type of optimization has no effect on the efficiency of the system. Especially since the compiler optimizer produces the same assembly output whether there is a local variable or not.

There is also an interest in debugging mode, in which one can change the return value of the function at runtime to change the branch one was supposed to take and thus test exotic error cases (for example).