FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Adding Borland C++ Builder 5 support. by dvader · Pull Request #137 · unittest-cpp/unittest-cpp · GitHub

/ unittest-cpp Public

Adding Borland C++ Builder 5 support. - #137

Open
dvader wants to merge 4 commits into
unittest-cpp:masterfrom
dvader:master
ConversationCommits (4)ChecksFiles changed
Open

Adding Borland C++ Builder 5 support.#137
dvader wants to merge 4 commits into
unittest-cpp:masterfrom
dvader:master

Conversation

dvader commented Nov 18, 2016

Copy link
Copy Markdown

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.

Adding Borland C++ Builder 5 support.
2809901 
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 commented Nov 19, 2016

Copy link
Copy Markdown
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 commented Nov 20, 2016

Copy link
Copy Markdown
Author

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 commented Nov 20, 2016

Copy link
Copy Markdown
Author

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
Build clean and passes all the tests except three using CMake.
bcf039e 
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.
Catching and resolving the exception thrown when accessing an empty s…
c5aa621 
…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 commented Nov 29, 2016

Copy link
Copy Markdown
Author

@pjohnmeyer @grahamreeds any comments or suggestions?

pjohnmeyer requested changes Jul 1, 2017
View reviewed changes

pjohnmeyer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread CMakeLists.txt


if(BORLAND)
set(UTPP_USE_PLUS_SIGN OFF)

pjohnmeyer Jul 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this set statement required for Borland? Can it not generate output files with + in the name?

Comment thread CMakeLists.txt
if(${UTPP_USE_PLUS_SIGN})
set_target_properties(UnitTest++ PROPERTIES OUTPUT_NAME UnitTest++)
else()
set_target_properties(UnitTest++ PROPERTIES OUTPUT_NAME UnitTestpp)

pjohnmeyer Jul 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Comment thread UnitTest++/MemoryOutStream.cpp
m_text = this->str();
try
{
m_text = this->str();

pjohnmeyer Jul 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread tests/Main.cpp
@@ -1,4 +1,7 @@
#include "UnitTest++/UnitTestPP.h"
#ifdef __BORLANDC__
#include <vcl.h>

pjohnmeyer Jul 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I didn't think we needed the Visual Component Library to run a console application?

Comment thread tests/TestCheckMacros.cpp

using namespace std;
#ifdef __BORLANDC__
using namespace UnitTest;

pjohnmeyer Jul 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are there individual statements that can be changed, rather than adding this?

Comment thread UnitTest++/MemoryOutStream.cpp
{
this->str(std::string());
m_text = this->str();
m_text = std::string();

pjohnmeyer Jul 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe m_text.clear() instead of another allocation?

Comment thread tests/TestMemoryOutStream.cpp

// 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&)

pjohnmeyer Jul 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

So, this test still fails, or did one of your other changes fix it?

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

pjohnmeyer pjohnmeyer requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL