MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0031514Open CASCADE[OCCT] OCCT:Foundation Classespublic2020-04-23 10:092020-05-03 07:25
Reportermsv 
Assigned Toabv 
PrioritynormalSeverityminor 
StatusfeedbackResolutionopen 
PlatformOSOS Version
Product Version[OCCT] 7.4.0 
Target Version[OCCT] 7.5.0*Fixed in Version 
Summary0031514: Foundation Classes - Add Base64 encoding function
DescriptionWe have FSD_Base64Decoder::Decode() that is to decode Base64 data.
It is needed to implement encoding function.
Probably, we need to rename FSD_Base64Decoder to FSD_Base64.
Steps To ReproduceN/A
TagsNo tags attached.
Test case numberNot required
Attached Files

- Relationships
related to 0031513newkgv Data Exchange - FSD_Base64Decoder::Decode() returns buffer with wrong length 
child of 0030692closedbugmaster Data Exchange - introduce base framework RWMesh for importing mesh data formats into XDE document 

-  Notes
(0091727)
git (administrator)
2020-04-23 13:12

Branch CR31514-ASRV has been created by msv.

SHA-1: 3d6ec4c72e5b661ea9206c575ee72ba8844bd985


Detailed log of new commits:

Author: msv
Date: Thu Apr 23 13:13:11 2020 +0300

    0031514: Foundation Classes - Add Base64 encoding function
    
    The class FSD_Base64Decoder has been renamed to FSD_Base64.
    The new method FSD_Base64::Encode has been added.
(0091729)
msv (developer)
2020-04-23 13:13

Dear Kirill, could you review the branch CR31514-ASRV?
(0091731)
git (administrator)
2020-04-23 13:26

Branch CR31514-ASRV has been updated forcibly by msv.

SHA-1: 315cb2a783015bd8d7edf34af8d0a5bb8a4b27d4
(0091732)
git (administrator)
2020-04-23 13:28

Branch CR31514 has been created by msv.

SHA-1: ddb4e6eacc88350abc4e976c1ab6887790f56444


Detailed log of new commits:

Author: msv
Date: Thu Apr 23 13:13:11 2020 +0300

    0031514: Foundation Classes - Add Base64 encoding function
    
    The class FSD_Base64Decoder has been renamed to FSD_Base64.
    The new method FSD_Base64::Encode has been added.
(0091733)
msv (developer)
2020-04-23 13:32

Tests are running.
http://jenkins-test-12.nnov.opencascade.com/view/CR31514-master-MSV/view/COMPARE/ [^]
(0091734)
git (administrator)
2020-04-23 14:04

Branch CR31514 has been updated forcibly by msv.

SHA-1: dd66989098f289c7ec228f0b9f29cce26e29d368
(0091736)
git (administrator)
2020-04-23 15:31

Branch CR31514-ASRV has been updated forcibly by msv.

SHA-1: 03101662cc289bdad04c193b94bb013c4d1d0a11
(0091737)
git (administrator)
2020-04-23 17:50

Branch CR31514-ASRV has been updated by msv.

SHA-1: c7a28bc0e089b4e717e1b229e3c4787613e48818


Detailed log of new commits:

Author: msv
Date: Thu Apr 23 17:51:41 2020 +0300

    #Change API

(0091740)
git (administrator)
2020-04-23 18:05

Branch CR31514_1 has been created by msv.

SHA-1: 7d909ac7b4a11cbe61007c18a1d8d900566f7ee3


Detailed log of new commits:

Author: msv
Date: Thu Apr 23 13:13:11 2020 +0300

    0031514: Foundation Classes - Add Base64 encoding function
    
    The class FSD_Base64Decoder has been renamed to FSD_Base64.
    The new method FSD_Base64::Encode has been added.
(0091741)
git (administrator)
2020-04-23 18:08

Branch CR31514-ASRV has been updated forcibly by msv.

SHA-1: c293fd6a9c31919e8e9114a25604e097754d5d3c
(0091742)
git (administrator)
2020-04-23 18:54

Branch CR31514_1 has been updated by msv.

SHA-1: 839c2da334f13276f6c7187fd11a96d3b1a2f9e3


Detailed log of new commits:

Author: msv
Date: Thu Apr 23 18:55:27 2020 +0300

    #Fix regression

(0091743)
git (administrator)
2020-04-23 18:57

Branch CR31514-ASRV has been updated forcibly by msv.

SHA-1: dd9f370110e5b0a231e2ff80cf3c0728ec1e0f7c
(0091744)
msv (developer)
2020-04-23 19:01

Please review again.
Tests are running.
http://jenkins-test-12.nnov.opencascade.com/view/CR31514-master-MSV/view/COMPARE/ [^]
(0091745)
msv (developer)
2020-04-23 19:01

The branch is CR31514_1.
(0091746)
kgv (developer)
2020-04-23 19:05

No more remarks - please raise the patch in OCCT branch CR31514_1.
(0091748)
git (administrator)
2020-04-23 19:13

Branch CR31514-ASRV has been updated by msv.

SHA-1: 400f8b49dd4e502188864602e601dc14e17ecb65


Detailed log of new commits:

Author: msv
Date: Thu Apr 23 19:14:06 2020 +0300

    #Eliminate compilation warning on clang

(0091749)
git (administrator)
2020-04-23 19:18

Branch CR31514_1 has been updated by msv.

SHA-1: 68ac72fb9d23a73a3e16ffd94cc9c700c0f158af


Detailed log of new commits:

Author: msv
Date: Thu Apr 23 19:14:06 2020 +0300

    #Eliminate compilation warning on clang

(0091750)
git (administrator)
2020-04-23 19:29

Branch CR31514-ASRV has been updated forcibly by msv.

SHA-1: 7d6964bb7d92225d19272d19ef1e5b346719e397
(0091766)
bugmaster (administrator)
2020-04-24 10:40

Combination -
OCCT branch : WEEK-17
master SHA - 03fce6fac0f286c86b6b5c49a43b094fc4a8ab07
a206de37fbfa0bf71bd534ae47192bbec23b8522
Products branch : WEEK-17 SHA - 30cd52f3d9c1f4ceaee9b46e90331b1e68a80d57
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 17149.45000000016 / 16936.94000000008 [+1.25%]
Products
Total CPU difference: 11292.350000000108 / 11283.690000000102 [+0.08%]
Windows-64-VC14:
OCCT
Total CPU difference: 18622.171875 / 18389.65625 [+1.26%]
Products
Total CPU difference: 13047.59375 / 13180.625 [-1.01%]


Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention
(0091795)
git (administrator)
2020-04-26 11:46

Branch CR31514_1 has been updated forcibly by msv.

SHA-1: 1fd51cf84255a8cc6e29e972ebfd9f841704931b
(0091796)
msv (developer)
2020-04-26 11:48

I have made agreed corrections. Please review.
(0091818)
git (administrator)
2020-04-27 11:07

Branch CR31514_2 has been created by msv.

SHA-1: 97d4c8dc172cd7ddce2e4ce8f73a511c5b28ca62


Detailed log of new commits:

Author: msv
Date: Thu Apr 23 13:13:11 2020 +0300

    0031514: Foundation Classes - Add Base64 encoding function
    
    The class FSD_Base64Decoder has been renamed to FSD_Base64.
    The new method FSD_Base64::Encode has been added.
    The method Decode has been improved for performance.
(0091819)
msv (developer)
2020-04-27 11:09

Please review again.
(0091828)
abv (manager)
2020-04-28 22:16

Please consider remarks:

1. Functions Encode() and Decode() take pointer to buffer of unspecified size and use it assuming implicitly that it will be sufficient for the data. There is no check for this, and that is quite dangerous. Please consider taking NCollection_Buffer or TColStd_(H)Array1OfByte (which both can either allocate their own buffer or attach to existing one).

2. Please follow OCCT conventions for names of local variables. I would suggest renaming "B64Map" to "aBase64Chars" and "B64Codes" to "aBase64Codes"

3. Please use consistent types: either Standard_Integer or int (see lines 46 vs 56). I would suggest using types with known size, such as int32_t (or int16_t, if appropriate), uint16_t etc. (size of int may differ between platforms)

4. In second FSD_Base64::Decode(), I believe generation of error message should be avoided - it is too low-level function to generate user messages directly. In case of insufficient memory the caller will know that by receiving null handle (this is the only case, null return for empty input buffer described in header is apparently wrong), and can generate the message as needed.

5. Please consider providing second implementation of Encode() returning NCollection_Buffer, for consistency with Decode()
(0091846)
msv (developer)
2020-04-29 11:52

1. Usage of NCollection_Buffer is inconvenient due to necessity to provide the allocator. I think it is better for such low-level method not to bind to any complex OCCT types. I will rather add the new length parameter for safety.

2. OK

3. OK

4. OK to get rid of generation of message. However, in the case of empty input buffer the method will return null. It is because NCollection_Buffer::Allocate will return 0 if requested size is 0.

By the way, there is strange code in that method:
    if (theSize != 0
    || !myAllocator.IsNull())
    {
      myData = (Standard_Byte* )myAllocator->Allocate (theSize);
    }
So, if theSize != 0 it will call myAllocator->Allocate even if myAllocator is null.

5. I will provide Encode() returning TCollection_AsciiString, as it is natural to use it rather than NCollection_Buffer for storing strings.
(0091847)
msv (developer)
2020-04-29 12:10

It would be good to create test cases for coding/decoding Base64.
(0091848)
git (administrator)
2020-04-29 12:14

Branch CR31514_2 has been updated by msv.

SHA-1: 3d80d4e6719bdce881577a5a1a303d98e489d161


Detailed log of new commits:

Author: msv
Date: Wed Apr 29 12:14:29 2020 +0300

    #considering remarks

(0091849)
msv (developer)
2020-04-29 12:14

Please review latest changes. I will add tests later.
(0091850)
abv (manager)
2020-04-29 12:28

I agree that NCollection_Buffer is ugly, but that is a chance to improve it:
- Use default OCCT Allocator if no one is provided
- Correctly treat externally provided buffer (either do not return it, or do that on explicit request)
- Either prohibit or implement correctly copy construction and assignment

I believe TCollection_AsciiString is better not to be used - here we actually work with byte buffer rather than with text string, and will likely need to access the data in a manner not directly supported by that class (such as get plain pointer).

On 4: I believe the difference is between returning Null NCollection_Buffer and returning NCollection_Buffer with Null data.
(0091853)
abv (manager)
2020-04-29 12:35

Besides, should not we have explicit methods to compute size for target buffer instead of using cryptic approach with calling main method with incomplete parameters? We do not develop for WinAPI where this approach was adopted.
(0091855)
msv (developer)
2020-04-29 13:02

Improvement of NCollection_Buffer is out of scope of this bug. If needed please add a new bug. And I probably won't have time to do in that direction.

TCollection_AsciiString is good for working with strings, and it has the methods to work with its content as a buffer: ctor with number of chars, and ToCString(). The latter returns const char*, and we can add a new method like std::string::data() to get access to non-const content.

(4) I did not understand your idea. What should the method return in which case?

The idea to use WinAPI manner was sounded by Kirill, and I followed his recommendations.
(0091857)
kgv (developer)
2020-04-29 13:15

> I agree that NCollection_Buffer is ugly, but that is a chance to improve it:
> - Use default OCCT Allocator if no one is provided
I suppose you mean adding default NCollection_Buffer constructor which will use Common allocator. Passing NULL to existing constructor should be kept as is, or it will break existing code wrapping external data by passing NULL allocator.

> - Either prohibit or implement correctly copy construction and assignment
I suppose copy/assignment should be disabled.

> Besides, should not we have explicit methods to compute size
Current approach was proposed not for mimicking WinAPI (it is not only WinAPI, by the way) but to provide clear interface with single entry instead of defining numerous methods with potentially tangled or duplicated logic (probably not that critical in case of base64 codec). I found this design elegant and commonly known.
(0091913)
abv (manager)
2020-04-30 17:23

@msv: 4) I did not understand your idea. What should the method return in which case?

My point was only that comment to 2nd method Decode in header file is not correct:

//! @return null handle if theLen is 0 or in case of out of memory condition.

Actual implementation is that if theLen is 0, you will get non-null handle to NCollection_Buffer object of null size, this is not the same as null handle (returned in case of memory allocation failure).
(0091914)
msv (developer)
2020-04-30 18:02

Actually, NCollection_Buffer::Allocate() returns false in case of the requested size is 0. So, all in all null handle will be returned.
(0091925)
abv (manager)
2020-05-01 22:29

I see your point, but still deem you are wrong. Please have a closer look at NCollection_Buffer::Allocate(): it has a bug in the "if" condition:

    if (theSize != 0
    || !myAllocator.IsNull())

Here we have || instead of &&, this means that for theSize == 0 allocator will be called (as soon as it is not null) and return non-zero buffer (note that allocator shall return non-zero pointer for requested block of zero size, following behavior of malloc()). Hence, the allocation shall succeed and return true.
(0091926)
msv (developer)
2020-05-02 12:16

I have already told about this bug above: see 0031514:0091846.
When it is fixed the description will be true ;)
Do you think the method Decode must return non-null empty buffer in case of input zero-size string?
(0091929)
abv (manager)
2020-05-03 07:25

Yes I do - it is commonly adopted behavior that empty buffer is valid object of 0 size, not a NULL object.

- Issue History
Date Modified Username Field Change
2020-04-23 10:09 msv New Issue
2020-04-23 10:09 msv Assigned To => msv
2020-04-23 10:21 kgv Relationship added child of 0030692
2020-04-23 13:12 git Note Added: 0091727
2020-04-23 13:13 msv Note Added: 0091729
2020-04-23 13:26 git Note Added: 0091731
2020-04-23 13:28 git Note Added: 0091732
2020-04-23 13:32 msv Note Added: 0091733
2020-04-23 13:33 msv Status new => assigned
2020-04-23 13:33 msv Assigned To msv => kgv
2020-04-23 13:33 msv Status assigned => resolved
2020-04-23 13:33 msv Steps to Reproduce Updated View Revisions
2020-04-23 14:04 git Note Added: 0091734
2020-04-23 15:31 git Note Added: 0091736
2020-04-23 17:50 git Note Added: 0091737
2020-04-23 18:05 git Note Added: 0091740
2020-04-23 18:08 git Note Added: 0091741
2020-04-23 18:54 git Note Added: 0091742
2020-04-23 18:57 git Note Added: 0091743
2020-04-23 19:01 msv Note Added: 0091744
2020-04-23 19:01 msv Note Added: 0091745
2020-04-23 19:05 kgv Note Added: 0091746
2020-04-23 19:05 kgv Assigned To kgv => bugmaster
2020-04-23 19:05 kgv Status resolved => reviewed
2020-04-23 19:13 git Note Added: 0091748
2020-04-23 19:18 git Note Added: 0091749
2020-04-23 19:29 git Note Added: 0091750
2020-04-24 10:40 bugmaster Note Added: 0091766
2020-04-24 10:40 bugmaster Status reviewed => tested
2020-04-24 10:43 bugmaster Test case number => Not required
2020-04-24 20:05 bugmaster Assigned To bugmaster => abv
2020-04-24 20:05 bugmaster Status tested => assigned
2020-04-26 11:46 git Note Added: 0091795
2020-04-26 11:48 msv Note Added: 0091796
2020-04-26 11:48 msv Status assigned => resolved
2020-04-27 09:41 msv Assigned To abv => msv
2020-04-27 09:41 msv Status resolved => assigned
2020-04-27 11:07 git Note Added: 0091818
2020-04-27 11:09 msv Note Added: 0091819
2020-04-27 11:09 msv Assigned To msv => abv
2020-04-27 11:09 msv Status assigned => resolved
2020-04-28 22:16 abv Note Added: 0091828
2020-04-28 22:16 abv Assigned To abv => msv
2020-04-28 22:16 abv Status resolved => assigned
2020-04-28 22:59 abv Relationship added related to 0031513
2020-04-29 11:52 msv Note Added: 0091846
2020-04-29 12:10 msv Note Added: 0091847
2020-04-29 12:14 git Note Added: 0091848
2020-04-29 12:14 msv Note Added: 0091849
2020-04-29 12:15 msv Assigned To msv => abv
2020-04-29 12:15 msv Status assigned => resolved
2020-04-29 12:28 abv Note Added: 0091850
2020-04-29 12:35 abv Note Added: 0091853
2020-04-29 12:35 abv Assigned To abv => msv
2020-04-29 12:35 abv Status resolved => feedback
2020-04-29 13:02 msv Note Added: 0091855
2020-04-29 13:02 msv Assigned To msv => abv
2020-04-29 13:15 kgv Note Added: 0091857
2020-04-30 17:23 abv Note Added: 0091913
2020-04-30 18:02 msv Note Added: 0091914
2020-05-01 22:29 abv Note Added: 0091925
2020-05-02 12:16 msv Note Added: 0091926
2020-05-03 07:25 abv Note Added: 0091929


Copyright © 2000 - 2020 MantisBT Team
Powered by Mantis Bugtracker