#1 A pointer can not be negative - ftp.cc:1269

Closed
opened 1 year ago by cethyel · 3 comments
cethyel commented 1 year ago

Here is a bug report from cppcheck:

[ftp.cc:1269]: (style) A pointer can not be negative so it is either pointless or an error to check if it is.

An other one from clang-5.0.2

ftp.cc:1269:22: error:

  ordered comparison between pointer and zero ('const char *' and 'int')

if(ftpResponse(-1) <= 0 || (m_iRespType != 2) )

From what I read here and there, a cast to a void pointer can be done.

if(ftpResponse(-1) <= (void*) 0 || (m_iRespType != 2) )

It builds fine, but does the software work as intended?

I’ve noticed that gcc-4.8.5 and clang-3.0 does build “as is” without complaining. I stepped upon this while converting to cmake since I use clang-5.0.2 for the building.

Here is a bug report from cppcheck: [ftp.cc:1269]: (style) A pointer can not be negative so it is either pointless or an error to check if it is. An other one from clang-5.0.2 ftp.cc:1269:22: error: ordered comparison between pointer and zero ('const char *' and 'int') if(ftpResponse(-1) <= 0 || (m_iRespType != 2) ) From what I read here and there, a cast to a void pointer can be done. if(ftpResponse(-1) <= (void*) 0 || (m_iRespType != 2) ) It builds fine, but does the software work as intended? I've noticed that gcc-4.8.5 and clang-3.0 does build "as is" without complaining. I stepped upon this while converting to cmake since I use clang-5.0.2 for the building.
SlavekB commented 1 year ago
Owner

Because ftpResponse returns char*, I think that in this case, the purpose is to verify if null has been returned. I would therefore expect the following adjustment:

if( (ftpResponse(-1) == null) || (m_iRespType != 2) )
Because `ftpResponse` returns `char*`, I think that in this case, the purpose is to verify if null has been returned. I would therefore expect the following adjustment: ``` if( (ftpResponse(-1) == null) || (m_iRespType != 2) ) ```
MicheleC commented 1 year ago
Owner

“void” is by definition a null type, so it makes no sense to use order comparison on that, e.g. <= used on a void variable is pretty much a mistake. I believe the best solution is as in tdelibs:

if(!ftpResponse(-1) || (m_iRespType != 2) )

It seems there is quite a bit of duplication between

tde/main/applications/tdeio-ftps/tdeio_ftps/ftp.cc

and

tde/main/tdelibs/tdeioslave/ftp/ftp.cc
"void" is by definition a null type, so it makes no sense to use order comparison on that, e.g. <= used on a void variable is pretty much a mistake. I believe the best solution is as in tdelibs: ``` if(!ftpResponse(-1) || (m_iRespType != 2) ) ``` It seems there is quite a bit of duplication between ``` tde/main/applications/tdeio-ftps/tdeio_ftps/ftp.cc ``` and ``` tde/main/tdelibs/tdeioslave/ftp/ftp.cc ```
SlavekB commented 1 year ago
Owner

Resolved - used solution identical to tdelibs.

Resolved - used solution identical to tdelibs.
SlavekB added this to the R14.0.6 release milestone 1 year ago
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
Cancel
Save
There is no content yet.