Archived issue #0026179

Coding rules - eliminate -Wdeprecated-declarations CLang warnings on tmpnam() usage

Open CASCADEOCCT:Codingclosed23 public notes

Search issues

Description

The following warnings should be eliminated:
occt/src/OSD/OSD_Directory.cxx:64:32: warning: 'tmpnam' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of tmpnam(3), it is highly recommended that you use mkstemp(3) instead. [-Wdeprecated-declarations]
Standard_CString        name = tmpnam(NULL);
                               ^
occt/src/OSD/OSD_File.cxx:255:15: warning: 'tmpnam' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of tmpnam(3), it is highly recommended that you use mkstemp(3) instead. [-Wdeprecated-declarations]
 char *name = tmpnam((char*) 0) ;
              ^



gcc 4.9.2 on Linux generates similar warning:
In function "OSD_Directory::BuildTemporary()':
OSD_Directory.cxx:(.text+0x17b): warning: the use of "tmpnam' is dangerous, better use "mkstemp'

Steps to reproduce

N/A

Public activity

23 archived notes

Participants are labeled by their role within this record.

01Commenter 2
Branch [archived branch] has been created by Participant.

[revision removed]


Detailed log of new commits:

Author: rkv
Date: Fri Oct 30 13:44:03 2015 +0300

    0026179: Coding rules - eliminate -Wdeprecated-declarations CLang warnings on tmpnam() usage
02Commenter 2
mkstemp() and mkdtemp() are used instead of tempnam().
03Author
+char                    name[] = "tmpXXXXXX";

the patch would not work as expected because temporary file will be created in current working directory.

Please check if these methods are used at all and consider removing them.
04Commenter 2
Branch [archived branch] has been updated forcibly by Participant.

[revision removed]
05Author
-OSD_File OSD_File::BuildTemporary(){
+void OSD_File::BuildTemporary(){

since this function is now method of class - it would be better to close current file within OSD_File instance before opening new one for consistency and safety.

+
+  Standard_EXPORT Standard_Integer ReadLine (TCollection_AsciiString& Buffer, const Standard_Integer NByte);

could you please add description of this method?

+Standard_Integer OSD_File::ReadLine (TCollection_AsciiString& Buffer, const Standard_Integer NByte) {
+  Standard_Integer NbyteRead;
+  ReadLine(Buffer, NByte, NbyteRead);
+  return NbyteRead;

Since this is a small wrapper over existing method - maybe it would be better to inline body into method definition.

06Commenter 2
Branch [archived branch] has been updated forcibly by Participant.

[revision removed]
07Author
> Branch [archived branch] has been updated forcibly by Participant.
please do not override the branches already sent to review.
The best practice is to push corrections as dedicated commits (useful to keep history and to revise small corrections) and to prepare new branch with squashed changes (for actual testing and integration).

+  Standard_EXPORT inline Standard_Integer ReadLine (

Standard_EXPORT should not be used within inline methods - please drop the keyword.

  TEST_RAISE(  "Close"  );
 
- CloseHandle (myFileHandle);
+ if (myFileHandle != INVALID_HANDLE_VALUE)
+  CloseHandle (myFileHandle);

there is no much use in this check since TEST_RAISE should throw exception in this case due to weird OSD_File design.

> since this function is now method of class
> - it would be better to close current file
> within OSD_File instance before opening new one for consistency and safety.
I don't see any changes related to this comment - OSD_File::BuildTemporary() still overrides myFileHandle, myFileChannel without any check.
I should at least throw exception as in OSD_File::Open():
> Standard_ProgramError::Raise("OSD_File::Open : file is already open")
08Commenter 2
Branch [archived branch] has been updated by Participant.

[revision removed]


Detailed log of new commits:

Author: rkv
Date: Tue Nov 10 12:00:30 2015 +0300

    0026179: Coding rules - eliminate -Wdeprecated-declarations CLang warnings on tmpnam() usage
    
    Drop Standard_EXPORT for inline method declaration.
    Remove unnecessary check when closing OSD_File.
    Close old file before building a new temporary file.

09Author
Please test the patch.
10Commenter 10
Dear Commenter 1,
Branch CR26179 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested on Release mode.
[revision removed]

Number of compiler warnings:

occt component :
Linux: 2 (0 on master)
Windows: 4 (0 on master)
MacOS : 146 (149 on master)

products component :
Linux: 39 (39 on master)
Windows: 0 (0 on master)

There are new additional compilation warnings on Linux platform:
http://jenkins-test-01.nnov.opencascade.com:8080/user/mnt/my-views/view/A_mnt_warnings/portlet/dashboard_portlet_17008/job/CR26179-master_build_occt_linux/1/warnings17Result/
Draw_Interpretor.cxx:201, GNU C Compiler 4 (gcc), Priority: Normal
'fd_err_save' may be used uninitialized in this function [-Wmaybe-uninitialized]
Draw_Interpretor.cxx:202, GNU C Compiler 4 (gcc), Priority: Normal
'fd_out_save' may be used uninitialized in this function [-Wmaybe-uninitialized]

There are new additional compilation warnings on Windows platform:
http://jenkins-test-01.nnov.opencascade.com:8080/user/mnt/my-views/view/A_mnt_warnings/portlet/dashboard_portlet_17008/job/CR26179-master_build_occt_windows_64/1/warnings34Result/
draw_interpretor.cxx:201, MSBuild, Priority: Normal
potentially uninitialized local variable 'fd_err_save' used [d:\builds\vc10\CR26179-master-occt-64\adm\msvc\vc10\TKDraw.vcxproj]
draw_interpretor.cxx:201, MSBuild, Priority: Normal
potentially uninitialized local variable 'fd_err_save' used
draw_interpretor.cxx:202, MSBuild, Priority: Normal
potentially uninitialized local variable 'fd_out_save' used [d:\builds\vc10\CR26179-master-occt-64\adm\msvc\vc10\TKDraw.vcxproj]
draw_interpretor.cxx:202, MSBuild, Priority: Normal
potentially uninitialized local variable 'fd_out_save' used

Regressions/Differences/Improvements:
http://occt-tests/CR26179-master-occt-64/Debian70-64/bugs/fclasses/bug670.html
bugs fclasses bug670: FAILED

Testing cases:
Not needed

Testing on Linux:
occt component :
Total MEMORY difference: 92132470 / 92249915 [-0.13%]
Total CPU difference: 19034.059999999987 / 19654.729999999847 [-3.16%]
products component :
Total MEMORY difference: 26224469 / 26273564 [-0.19%]
Total CPU difference: 7571.0499999999965 / 7540.919999999997 [+0.40%]

Testing on Windows:
occt component :
Total MEMORY difference: 58095356 / 58098202 [-0.00%]
Total CPU difference: 18347.99521459888 / 18063.496190898888 [+1.57%]
products component :
Total MEMORY difference: 17163441 / 17166598 [-0.02%]
Total CPU difference: 5739.479591299958 / 5624.304052999975 [+2.05%]

There are no differences in images found by testdiff.
11Commenter 11
Dear rkv,
Branch [archived branch] has been rejected due to:
- additional warnings
- regressions/differences/improvements
12Commenter 2
Branch [archived branch] has been updated by Participant.

[revision removed]


Detailed log of new commits:

Author: rkv
Date: Fri Nov 13 11:56:41 2015 +0300

    0026179: Coding rules - eliminate -Wdeprecated-declarations CLang warnings on tmpnam() usage
    
    Eliminate warnings about uninitialized local variable.

13Commenter 2
Branch [archived branch] has been updated forcibly by Participant.

[revision removed]
14Author
Dear rkv,

please provide description of last corrections - either in git or at least here in bugtracker.
15Commenter 15
There is the comment #48133:
"Eliminate warnings about uninitialized local variable."

- appropriate variables are initialized now.
16Author
Dear rkv,

test report includes
> bugs fclasses bug670: FAILED
and your comment does not specify if this regression has been fixed or not and how.

17Commenter 17
According to apv the test case OCC670 "is not stable" and as it is agreed by Andrey Betenev to run tests again.

18Author
Please test updated patch.

> According to apv the test case OCC670 "is not stable"
thank you for explanation - it should be here in bugtracker so that others will be able to understand why this regression has been ignored.

19Commenter 2
Branch [archived branch] has been updated forcibly by Participant.

[revision removed]
20Commenter 20
Branch [archived branch] has been rebased on the current master
21Commenter 2
Branch [archived branch] has been updated by Participant.

[revision removed]


Detailed log of new commits:

Author: abv
Date: Thu Nov 19 15:20:39 2015 +0300

    0000670: Problem with the Print method.
    
    Test for issue #0000670 corrected to avoid unpredictable behavior

22Commenter 22
Dear Commenter 1,

Branch CR26179 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested.
[revision removed]

Number of compiler warnings:
occt component:
   Linux: 0 (0 on master)
   Windows: 0 (0 on master)
   MacOS: 127 (129 on master)
products component:
   Linux: 39 (39 on master)
   Windows: 0 (0 on master)

Regressions/Differences:
Not detected

Testing cases:
Not needed

Testing on Linux:
Total MEMORY difference: 92823781 / 92917230 [-0.10%]
Total CPU difference: 19858.489999999852 / 19784.59999999988 [+0.37%]

Testing on Windows:
Total MEMORY difference: 58081568 / 58096361 [-0.03%]
Total CPU difference: 18516.601095399234 / 18232.414073698874 [+1.56%]
23Commenter 2
Branch [archived branch] has been deleted by Author.

[revision removed]