Permalink
Browse files

Eliminated objects that hold constant strings. Aids step-debugging in…

… MSVC
  • Loading branch information...
1 parent 1fe4595 commit aef47282ecf147957222381496158ca3016b0ed0 @cdmh committed Oct 12, 2013
@@ -16,14 +16,14 @@ namespace Catch {
struct AssertionInfo
{
AssertionInfo() {}
@PureAbstract
PureAbstract Oct 12, 2013

change to const char * means macroName and capturedExpression are no longer initialised here.

- AssertionInfo( std::string const& _macroName,
+ AssertionInfo( char const * _macroName,
SourceLineInfo const& _lineInfo,
- std::string const& _capturedExpression,
+ char const * _capturedExpression,
ResultDisposition::Flags _resultDisposition );
- std::string macroName;
+ char const * macroName;
SourceLineInfo lineInfo;
- std::string capturedExpression;
+ char const * capturedExpression;
ResultDisposition::Flags resultDisposition;
};
@@ -53,7 +53,7 @@ namespace Catch {
std::string getExpandedExpression() const;
std::string getMessage() const;
SourceLineInfo getSourceInfo() const;
- std::string getTestMacroName() const;
+ char const * getTestMacroName() const;
protected:
AssertionInfo m_info;
@@ -13,9 +13,9 @@
namespace Catch {
- AssertionInfo::AssertionInfo( std::string const& _macroName,
+ AssertionInfo::AssertionInfo( char const * _macroName,
SourceLineInfo const& _lineInfo,
- std::string const& _capturedExpression,
+ char const * _capturedExpression,
ResultDisposition::Flags _resultDisposition )
: macroName( _macroName ),
lineInfo( _lineInfo ),
@@ -47,7 +47,7 @@ namespace Catch {
}
bool AssertionResult::hasExpression() const {
- return !m_info.capturedExpression.empty();
+ return m_info.capturedExpression != 0 && *m_info.capturedExpression != 0;
@PureAbstract
PureAbstract Oct 12, 2013

capturedExpression != 0 should be spelt capturedExpression != nullptr (or, better still, just capturedExpression)
so replace with return m_info.capturedExpression && *m_info.capturedExpression

}
bool AssertionResult::hasMessage() const {
@@ -56,15 +56,15 @@ namespace Catch {
std::string AssertionResult::getExpression() const {
if( shouldNegate( m_info.resultDisposition ) )
- return "!" + m_info.capturedExpression;
+ return std::string("!") + m_info.capturedExpression;
else
return m_info.capturedExpression;
}
std::string AssertionResult::getExpressionInMacro() const {
- if( m_info.macroName.empty() )
+ if( m_info.macroName == 0 || *m_info.macroName == 0)
@PureAbstract
PureAbstract Oct 12, 2013

macroName == 0 should be macroName == nullptr

return m_info.capturedExpression;
else
- return m_info.macroName + "( " + m_info.capturedExpression + " )";
+ return std::string(m_info.macroName) + "( " + m_info.capturedExpression + " )";
@PureAbstract
PureAbstract Oct 12, 2013

stringstream is a far more efficient want of concatenating strings than artificially creating a std::string to do so. (The old code already had one to do that with)

}
bool AssertionResult::hasExpandedExpression() const {
@@ -82,7 +82,7 @@ namespace Catch {
return m_info.lineInfo;
}
- std::string AssertionResult::getTestMacroName() const {
+ char const *AssertionResult::getTestMacroName() const {
return m_info.macroName;
}
@@ -105,7 +105,7 @@ namespace Catch {
struct SourceLineInfo {
SourceLineInfo() : line( 0 ){}
@PureAbstract
PureAbstract Oct 12, 2013

Changing file to const char * means that it will no longer be initialised here - I'd suggest intialising with nullptr

- SourceLineInfo( std::string const& _file, std::size_t _line )
+ SourceLineInfo( char const * _file, std::size_t _line )
: file( _file ),
line( _line )
{}
@@ -114,12 +114,12 @@ namespace Catch {
line( other.line )
{}
bool empty() const {
- return file.empty();
+ return file == 0 || *file == 0;
@PureAbstract
PureAbstract Oct 12, 2013

file == 0 should be spelt file == nullptr.

}
bool operator == ( SourceLineInfo const& other ) const {
- return line == other.line && file == other.file;
+ return line == other.line && ((empty() && other.empty()) || strcmp(file, other.file) == 0);
}
- std::string file;
+ char const * file;
std::size_t line;
};
@@ -90,7 +90,7 @@ namespace Catch {
return m_exprComponents.lhs + "\n" + m_exprComponents.op + "\n" + m_exprComponents.rhs;
}
else
- return "{can't expand - use " + info.macroName + "_FALSE( " + info.capturedExpression.substr(1) + " ) instead of " + info.macroName + "( " + info.capturedExpression + " ) for better diagnostics}";
+ return "{can't expand - use " + std::string(info.macroName) + "_FALSE( " + *(info.capturedExpression+1) + " ) instead of " + info.macroName + "( " + info.capturedExpression + " ) for better diagnostics}";
}
} // end namespace Catch
@@ -15,11 +15,11 @@
namespace Catch {
struct MessageInfo {
- MessageInfo( std::string const& _macroName,
+ MessageInfo( char const * _macroName,
SourceLineInfo const& _lineInfo,
ResultWas::OfType _type );
- std::string macroName;
+ char const * macroName;
SourceLineInfo lineInfo;
ResultWas::OfType type;
std::string message;
@@ -36,7 +36,7 @@ namespace Catch {
};
struct MessageBuilder {
- MessageBuilder( std::string const& macroName,
+ MessageBuilder( char const * macroName,
SourceLineInfo const& lineInfo,
ResultWas::OfType type )
: m_info( macroName, lineInfo, type )
@@ -12,7 +12,7 @@
namespace Catch {
- MessageInfo::MessageInfo( std::string const& _macroName,
+ MessageInfo::MessageInfo( char const * _macroName,
SourceLineInfo const& _lineInfo,
ResultWas::OfType _type )
: macroName( _macroName ),

3 comments on commit aef4728

@PureAbstract

There are a few places where this looks like it breaks - I've commented individually on a few of them, but there are too many of them for me to be comfortable with this patch.

@cdmh
Owner
cdmh commented on aef4728 Oct 12, 2013

Good catch on the two default ctors, I'll fix those.

I don't see other breaking changes. "too many", really? your other comments are on style, or minor performance. Catch isn't high performance anyway, so I don't think that's a valid criticism. Added stringstreams for concatenation seems overkill to me. With SSO, I suspect performance isn't greatly affected, although I haven't profiles. the code.

@PureAbstract

I spotted a couple of places that this patch breaks - that doesn't mean I spotted all the places that it breaks. So yes, for me, that's too many.
"I don't see any other breaking changes" is not the same thing as "there are no other breaking changes.".

The current mechanism (using std::string) has been around for a long while, and has had a lot of 'field testing'. I've not heard a convincing argument that changing it is worthwhile. (I don't consider "MSVC's debugger sucks" to be a convincing argument).

Please sign in to comment.