View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0031514 | Open CASCADE | OCCT:Foundation Classes | public | 2020-04-23 10:09 | 2021-09-05 23:28 |
Reporter | Assigned To | bugmaster | |||
Priority | normal | Severity | minor | ||
Status | closed | Resolution | fixed | ||
Product Version | 7.4.0 | ||||
Target Version | 7.6.0 | Fixed in Version | 7.6.0 | ||
Summary | 0031514: Foundation Classes - Add Base64 encoding function | ||||
Description | We 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 Reproduce | N/A | ||||
Tags | No tags attached. | ||||
Test case number | Not required | ||||
|
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. |
|
Dear Kirill, could you review the branch CR31514-ASRV? |
|
Branch CR31514-ASRV has been updated forcibly by msv. SHA-1: 315cb2a783015bd8d7edf34af8d0a5bb8a4b27d4 |
|
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. |
|
Tests are running. http://jenkins-test-12.nnov.opencascade.com/view/CR31514-master-MSV/view/COMPARE/ |
|
Branch CR31514 has been updated forcibly by msv. SHA-1: dd66989098f289c7ec228f0b9f29cce26e29d368 |
|
Branch CR31514-ASRV has been updated forcibly by msv. SHA-1: 03101662cc289bdad04c193b94bb013c4d1d0a11 |
|
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 |
|
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. |
|
Branch CR31514-ASRV has been updated forcibly by msv. SHA-1: c293fd6a9c31919e8e9114a25604e097754d5d3c |
|
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 |
|
Branch CR31514-ASRV has been updated forcibly by msv. SHA-1: dd9f370110e5b0a231e2ff80cf3c0728ec1e0f7c |
|
Please review again. Tests are running. http://jenkins-test-12.nnov.opencascade.com/view/CR31514-master-MSV/view/COMPARE/ |
|
The branch is CR31514_1. |
|
No more remarks - please raise the patch in OCCT branch CR31514_1. |
|
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 |
|
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 |
|
Branch CR31514-ASRV has been updated forcibly by msv. SHA-1: 7d6964bb7d92225d19272d19ef1e5b346719e397 |
|
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 |
|
Branch CR31514_1 has been updated forcibly by msv. SHA-1: 1fd51cf84255a8cc6e29e972ebfd9f841704931b |
|
I have made agreed corrections. Please review. |
|
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. |
|
Please review again. |
|
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() |
|
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. |
|
It would be good to create test cases for coding/decoding Base64. |
|
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 |
|
Please review latest changes. I will add tests later. |
|
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. |
|
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. |
|
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. |
|
> 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. |
|
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). |
|
Actually, NCollection_Buffer::Allocate() returns false in case of the requested size is 0. So, all in all null handle will be returned. |
|
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. |
|
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? |
|
Yes I do - it is commonly adopted behavior that empty buffer is valid object of 0 size, not a NULL object. |
|
Branch CR31514_3 has been created by kgv. SHA-1: c3a4418696a5cbb67daf3fa042ed75852bed3858 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. |
|
Please raise the patch - OCCT: branch CR31514_3. http://jenkins-test-12.nnov.opencascade.com:8080/view/CR31514_3-master-KGV/ |
|
Combination - OCCT branch : IR-2021-08-27 master SHA - 8f70959571ab8999b47f88d673343587e733f364 a87b7ddc8cb44606b91e3f37113847c3f5f50fdc Products branch : IR-2021-08-27 SHA - 71e8b69563894a6e7348c9bd5b0294a5e5e8fe3f 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: 17432.060000000307 / 17364.750000000335 [+0.39%] Products Total CPU difference: 11534.270000000102 / 11550.27000000011 [-0.14%] Windows-64-VC14: OCCT Total CPU difference: 19189.15625 / 19243.171875 [-0.28%] Products Total CPU difference: 12874.53125 / 12885.734375 [-0.09%] Image differences : No differences that require special attention Memory differences : No differences that require special attention |
|
Branch CR31514 has been deleted by mnt. SHA-1: dd66989098f289c7ec228f0b9f29cce26e29d368 |
|
Branch CR31514_1 has been deleted by mnt. SHA-1: 1fd51cf84255a8cc6e29e972ebfd9f841704931b |
|
Branch CR31514_2 has been deleted by mnt. SHA-1: 3d80d4e6719bdce881577a5a1a303d98e489d161 |
|
Branch CR31514_3 has been deleted by mnt. SHA-1: c3a4418696a5cbb67daf3fa042ed75852bed3858 |
|
Branch CR31514-ASRV has been deleted by kgv. SHA-1: 7d6964bb7d92225d19272d19ef1e5b346719e397 |
occt: master 45d498ef 2020-04-23 10:13:11
Committer: bugmaster Details Diff |
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. |
Affected Issues 0031514 |
|
mod - src/FSD/FILES | Diff File | ||
add - src/FSD/FSD_Base64.cxx | Diff File | ||
add - src/FSD/FSD_Base64.hxx | Diff File | ||
rm - src/FSD/FSD_Base64Decoder.cxx | Diff File | ||
rm - src/FSD/FSD_Base64Decoder.hxx | Diff File | ||
mod - src/RWGltf/RWGltf_GltfJsonParser.cxx | Diff File |
Date Modified | Username | Field | Change |
---|---|---|---|
2020-04-23 10:09 |
|
New Issue | |
2020-04-23 10:09 |
|
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 |
|
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 |
|
Note Added: 0091733 | |
2020-04-23 13:33 |
|
Status | new => assigned |
2020-04-23 13:33 |
|
Assigned To | msv => kgv |
2020-04-23 13:33 |
|
Status | assigned => resolved |
2020-04-23 13:33 |
|
Steps to Reproduce Updated | |
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 |
|
Note Added: 0091744 | |
2020-04-23 19:01 |
|
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 |
|
Note Added: 0091796 | |
2020-04-26 11:48 |
|
Status | assigned => resolved |
2020-04-27 09:41 |
|
Assigned To | abv => msv |
2020-04-27 09:41 |
|
Status | resolved => assigned |
2020-04-27 11:07 | git | Note Added: 0091818 | |
2020-04-27 11:09 |
|
Note Added: 0091819 | |
2020-04-27 11:09 |
|
Assigned To | msv => abv |
2020-04-27 11:09 |
|
Status | assigned => resolved |
2020-04-28 22:16 |
|
Note Added: 0091828 | |
2020-04-28 22:16 |
|
Assigned To | abv => msv |
2020-04-28 22:16 |
|
Status | resolved => assigned |
2020-04-28 22:59 |
|
Relationship added | related to 0031513 |
2020-04-29 11:52 |
|
Note Added: 0091846 | |
2020-04-29 12:10 |
|
Note Added: 0091847 | |
2020-04-29 12:14 | git | Note Added: 0091848 | |
2020-04-29 12:14 |
|
Note Added: 0091849 | |
2020-04-29 12:15 |
|
Assigned To | msv => abv |
2020-04-29 12:15 |
|
Status | assigned => resolved |
2020-04-29 12:28 |
|
Note Added: 0091850 | |
2020-04-29 12:35 |
|
Note Added: 0091853 | |
2020-04-29 12:35 |
|
Assigned To | abv => msv |
2020-04-29 12:35 |
|
Status | resolved => feedback |
2020-04-29 13:02 |
|
Note Added: 0091855 | |
2020-04-29 13:02 |
|
Assigned To | msv => abv |
2020-04-29 13:15 | kgv | Note Added: 0091857 | |
2020-04-30 17:23 |
|
Note Added: 0091913 | |
2020-04-30 18:02 |
|
Note Added: 0091914 | |
2020-05-01 22:29 |
|
Note Added: 0091925 | |
2020-05-02 12:16 |
|
Note Added: 0091926 | |
2020-05-03 07:25 |
|
Note Added: 0091929 | |
2020-09-25 20:35 |
|
Target Version | 7.5.0 => 7.6.0 |
2021-08-24 00:06 | git | Note Added: 0103381 | |
2021-08-24 01:34 | kgv | Note Added: 0103401 | |
2021-08-24 01:34 | kgv | Assigned To | abv => bugmaster |
2021-08-24 01:34 | kgv | Status | feedback => reviewed |
2021-08-28 15:43 | bugmaster | Note Added: 0103470 | |
2021-08-28 15:43 | bugmaster | Status | reviewed => tested |
2021-08-28 15:51 | bugmaster | Changeset attached | => occt master 45d498ef |
2021-08-28 15:51 | bugmaster | Status | tested => verified |
2021-08-28 15:51 | bugmaster | Resolution | open => fixed |
2021-08-28 15:59 | git | Note Added: 0103477 | |
2021-08-28 15:59 | git | Note Added: 0103478 | |
2021-08-28 15:59 | git | Note Added: 0103479 | |
2021-08-28 15:59 | git | Note Added: 0103480 | |
2021-09-05 23:28 | git | Note Added: 0103839 |