Skip to content

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dvader
Copy link

@dvader dvader commented Nov 18, 2016

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.

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.
@grahamreeds
Copy link
Contributor

Hmm. Have you tested this on gcc and msvc? I am not a fan of using namespace.

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
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.

You can view, comment on, or merge this pull request online at:

#137
Commit Summary

  • Adding Borland C++ Builder 5 support.

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#137, or mute the thread
https://github.com/notifications/unsubscribe-auth/AEy9nD0cKBkHCvzlZo-XLfXfvZE14MwFks5q_icmgaJpZM4K2_n6
.

@dvader
Copy link
Author

dvader commented Nov 20, 2016

The build passed for gcc, clang and msvc including all the unit tests. I've added some using namespace but there were some already.

@dvader
Copy link
Author

dvader commented Nov 20, 2016

I have added some conditional compiles around the using namespace UnitTest as the tests don't compile with the Borland compiler without them.
The using namespace are only in the unit test files and none in the actual library as the library compiles cleanly without them.

Alexis Denis added 2 commits November 21, 2016 16:24
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!!!
@dvader
Copy link
Author

dvader commented Nov 29, 2016

@pjohnmeyer @grahamreeds any comments or suggestions?

Copy link
Member

@pjohnmeyer pjohnmeyer left a 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)
Copy link
Member

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)
Copy link
Member

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();
Copy link
Member

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>
Copy link
Member

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;
Copy link
Member

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();
Copy link
Member

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&)
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants