-
Notifications
You must be signed in to change notification settings - Fork 179
Adding Borland C++ Builder 5 support. #137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Included the project files as well as the automatically generated makefiles. The library compiles without issues. Borland requires the UnitTest namespace to be defined at the beginning of tests. Added an default to have no support for C++11. Added a Borland to the list of compilers that don't recognize "long long" but use __int64 instead.
Hmm. Have you tested this on gcc and msvc? I am not a fan of Sent from my Nexus 6P On 18 Nov 2016 10:31 pm, "dvader" notifications@github.com wrote: Included the project files as well as the automatically generated but use __int64 instead.You can view, comment on, or merge this pull request online at: #137
File Changes
Patch Links:
— |
The build passed for gcc, clang and msvc including all the unit tests. I've added some using namespace but there were some already. |
I have added some conditional compiles around the |
cd builds cmake -G"Borland Makefiles" .. make Two of the tests fail because of unhandled floating point exceptions (throws when doing calculations using Nan). This was fixed by ignoring floating point exceptions. One test fails because std::string is not implemented properly in Borland C++ 5. This test is bypassed if using the Borland compiler. It does mean though that MemoryOutStream.Clear() doesn't work. A more permanent fix might be needed. I noticed a separate implementation of MemoryOutStream for VC6 that doesn't use std::string. Maybe we can use that one instead.
…tring from an ostrstream (MemoryOutStream GetString would throw if the stream was empty). Moving the exception disable to the test file instead of the main file so the workaround is located where it is needed. All tests pass!!!
@pjohnmeyer @grahamreeds any comments or suggestions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the PR. See individual comments. Most important:
- we may need to add another build option or change the documentation for
UTPP_USE_PLUS_SIGN
- please take a second look at MemoryOutStream changes
- I think we can reduce the Borland-specific lib/header dependence
|
||
|
||
if(BORLAND) | ||
set(UTPP_USE_PLUS_SIGN OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this set
statement required for Borland? Can it not generate output files with +
in the name?
@@ -59,28 +66,34 @@ add_library(UnitTest++ STATIC ${headers_} ${sources_} ${platformHeaders_} ${plat | |||
|
|||
if(${UTPP_USE_PLUS_SIGN}) | |||
set_target_properties(UnitTest++ PROPERTIES OUTPUT_NAME UnitTest++) | |||
else() | |||
set_target_properties(UnitTest++ PROPERTIES OUTPUT_NAME UnitTestpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me this is changing the meaning of UTPP_USE_PLUS_SIGN
; in its current incarnation it only changes the install path to use PP
instead of ++
. Am I right?
m_text = this->str(); | ||
try | ||
{ | ||
m_text = this->str(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the Borland implementation of ostringstream::str
throw if nothing has been streamed in yet?
If so, can we catch a more specific exception? Maybe std::exception
?
Or, can we change the exception mask to prevent the exception entirely? http://en.cppreference.com/w/cpp/io/basic_ios/exceptions
@@ -1,4 +1,7 @@ | |||
#include "UnitTest++/UnitTestPP.h" | |||
#ifdef __BORLANDC__ | |||
#include <vcl.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think we needed the Visual Component Library to run a console application?
@@ -4,6 +4,9 @@ | |||
#include "ScopedCurrentTest.h" | |||
|
|||
using namespace std; | |||
#ifdef __BORLANDC__ | |||
using namespace UnitTest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there individual statements that can be changed, rather than adding this?
return m_text.c_str(); | ||
} | ||
|
||
void MemoryOutStream::Clear() | ||
{ | ||
this->str(std::string()); | ||
m_text = this->str(); | ||
m_text = std::string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe m_text.clear()
instead of another allocation?
@@ -257,6 +257,9 @@ namespace { | |||
CHECK_EQUAL("53124", stream.GetText()); | |||
} | |||
|
|||
// Not sure why but this test fails with the Borland C++ 5.5 compiler. | |||
// It throws an unhandled exception: | |||
// unexpected NULL pointer in function: basic_string( const charT*,size_type,const Allocator&) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this test still fails, or did one of your other changes fix it?
Included the project files as well as the automatically generated makefiles. There is probably a better organization for all those files. Also CMake might be able to handle BCB5. I need help in this matter.
The library compiles without issues.
Borland requires the UnitTest namespace to be defined at the beginning of tests.
Added an default to have no support for C++11.
Added a Borland to the list of compilers that don't recognize "long long" but use __int64 instead.