Resolving build issues for some platforms #229
Conversation
dklimkin
changed the title
|
Agreed std::vector is a better option for dtntunnel. For AES128Stream yes, it's the same, these objects are heavy with all the buffers. Given this is in tests, I suggest the pointer. Any exception will break the test and abort anyway, so missing delete is not an issue. Again, I'd prefer std::unique_ptr here. |
- missing header for stand alone complitation, - error: stack frame size of N bytes in functions 'CipherStreamTest::aesstream_test01' [-Werror,-Wframe-larger-than=] 'TUN2BundleGateway::process' [-Werror,-Wframe-larger-than=] 'TUN2BundleGateway::received' [-Werror,-Wframe-larger-than=]
error: 'dtn::storage::DataStorage::Callback' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor]. error: 'dtn::daemon::Configuration::Extension' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor] error: 'dtn::daemon::Configuration::OnChangeListener' has virtual functions but non-virtual destructor [-Werror,-Wnon-virtual-dtor]
In IBR-DTN, I try to avoid C++11/14/17 due to incompatibility with very old compilers. That is the reason why, we use our own reference counting object (refcnt_ptr) as part of ibrcommon. It works the same way as std::unique_ptr. => |
|
Does it make sense here for a test? Not a big deal but seems to be an overkill. As per C++11/14/17, what compilers are you targetting? Any reason for using those old compilers? I understand not utilizing C+17 but C+11 is 6+ years old. |
|
IBR-DTN is also 6+ years old. The original goal was to run on very old/restricted embedded devices were GCC Version <= 4.0 was required. Since IBR-DTN is working fine, I do not like to change that because of some programmers convenience. |
|
Running it on old devices doesn't automatically mean compiling it with an old GCC... One can cross-compile from a modern desktop onto the target platform, the resulting code should be fully compatible. |
|
If the compiler is proprietary, it is necessary to use the old one. However, why we should change the codebase except for convenience? |
The point about old proprietary compiler does stand though. But I wonder who would now still use those, and why. True, I am not aware of the current user base of ibr-dtn. |
|
I understand that C++ with more built-in features is nicer and less error-prone for many developer. Your not the first one asking to contibute code in C++11 and above. If we want to break the condition of sticking to C++98, we have to rethink about the abstraction library ibrcommon. To make that step clean, a lot of code should refactored. A huge work which has to be checked thoroughly. Since IBR-DTN is now my free-time project and no longer something I'm paid for, I do not have the time refactor the code, or review this amount of code, or check if all tiny target devices still work with it. The main target of IBR-DTN is OpenWrt which has quite old compilers in it. Moreover, it is unclear if the ulibc supports the extensions you suggest. For that reason, I'm feeling more comfortable if we stick to the quite old but stable C++98 standard. |
|
Thank you for clarifying and for setting the expectations for the future changes. I suggest we discuss separately what are the options for such refactoring (e.g. on a branch?), if there are any at all. May be within #228 ? For this PR, do you insist on |
|
Yes. Use refcnt_ptr for now. Once if someone refactors the code for C++11, he can easily replace it with a text-search. This would be more complicated to find if you switch to raw pointers. |
| @@ -279,16 +280,17 @@ void CipherStreamTest::aesstream_test01() | |||
|
|
|||
| // encrypt the test data | |||
| { | |||
| ibrcommon::AES128Stream crypt_stream(ibrcommon::CipherStream::CIPHER_ENCRYPT, data, key, salt); | |||
| refcnt_ptr<ibrcommon::AES128Stream> crypt_stream = refcnt_ptr<ibrcommon::AES128Stream>( | |||
| new ibrcommon::AES128Stream(ibrcommon::CipherStream::CIPHER_ENCRYPT, data, key, salt)); | |||
There was a problem hiding this comment.
Is there a reason why you explicitly instantiate and assign the object instead of a simple initialization?
refcnt_ptr<ibrcommon::AES128Stream> crypt_stream(
new ibrcommon::AES128Stream(ibrcommon::CipherStream::CIPHER_ENCRYPT, data, key, salt));
There was a problem hiding this comment.
No reason other than writing Java and C++ code on the same day...
| @@ -299,13 +301,14 @@ void CipherStreamTest::aesstream_test01() | |||
|
|
|||
| // decrypt the test data | |||
| { | |||
| ibrcommon::AES128Stream crypt_stream(ibrcommon::CipherStream::CIPHER_DECRYPT, data, key, salt, iv); | |||
| refcnt_ptr<ibrcommon::AES128Stream> crypt_stream = refcnt_ptr<ibrcommon::AES128Stream>( | |||
| new ibrcommon::AES128Stream(ibrcommon::CipherStream::CIPHER_DECRYPT, data, key, salt, iv)); | |||
There was a problem hiding this comment.
Same as above. IMHO simple initialization would be better.
|
btw, my compiler spotted |
|
Oh no, That must have passed me somehow. ;-) This type comes from the C99 standard which is supported by even "old" compilers. Maybe that is the reason why I didn't noticed any issue with it before. |
'TUN2BundleGateway::process' [-Werror,-Wframe-larger-than=]
'TUN2BundleGateway::received' [-Werror,-Wframe-larger-than=]
'CipherStreamTest::aesstream_test01' [-Werror,-Wframe-larger-than=]