Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

g_oss is causing incorrect stringification results #567

Closed
Saalvage opened this issue Dec 11, 2021 · 3 comments
Closed

g_oss is causing incorrect stringification results #567

Saalvage opened this issue Dec 11, 2021 · 3 comments

Comments

@Saalvage
Copy link
Member

Saalvage commented Dec 11, 2021

Edit: I have now published a possible fix at #569!

I'm currently working on a stringification utility for STL classes, and I've discovered an obscure bug that manifests in two slightly different ways:

1.

When an argument to a MessageBuilder is stringified via being written to an ostream (operator<<) the result of that stringification is accidentally prepended to the result of the MessageBuilder.
Example:

struct A { }; struct B { };
static std::ostream& operator<<(std::ostream& o, const A&) { o << "A"; return o; }
template<> struct doctest::StringMaker<B> {
	static String convert(const B&) { return toString(A()) + "B"; }
};
TEST_CASE("") { FAIL(B()); }

Expected result: AB
Actual result: AAB

This can be avoided by first writing the String into an object like so:

doctest::String s = doctest::toString(B());
FAIL(s);

2.

When an operator<< utilizes toString which in turn executes its function via writing to a stream.
Arguably this is a bit contrived, since you usually wouldn't use toString in an operator<< overload, but it still shows a weakness in design, which demonstrates that simply giving MessageBuilderss their own thread local osstream won't solve this problem entirely.
There might be a less contrived instance of this bug lurking somewhere!
Example:

struct A { }; struct B { };
static std::ostream& operator<<(std::ostream& o, const A&) { o << "A"; return o; }
static std::ostream& operator<<(std::ostream& o, const B&) { o << "C" << "B" + doctest::toString(A()); return o; }
TEST_CASE("") { String s = toString(B()); FAIL(s); /* Failure is not due to MessageBuilder! */ }

Expected result: CBA
Actual result: ACA

The stream that is used to stringify B needs to be used again to stringify A, this messes things up.

I'm honestly not entirely sure on how to solve this, I think the idea of having a single string buffer will need to be abandoned.
My first approach was simply moving the logic of resetting the buffer into the result function, this solves 1. but modifies 2.'s result to be BCA, which is still incorrect!

My current progress can be found on https://github.com/Saalvage/doctest/tree/tlss
Some input would be appreciated!

Extra information

  • doctest version: a384864 (dev head at the time of posting)
  • Operating System: Windows 10 Pro 19043
  • Compiler+version: MSVC 22 17.0.2
@onqtam
Copy link
Member

onqtam commented Dec 15, 2021

So I fixed the MSVC 2015 builds and the obvious bugs in dev, but your AB vs AAB issue persisted. So I made a fix for it in another branch - take a look: 4f3591f

Basically, I changed it so that whenever getTlsOssResult gets called the stream gets cleaned (I had missed the use of getTlsOss in the MessageBuilder when fixing the dev branch), and there's no more cleaning going on in getTlsOss. Now your first example returns AB properly (in branch test_567).

However the second example still behaves weirdly, but if I change "B" + doctest::toString(A()) to "B" << doctest::toString(A()) it starts behaving properly - so there's something going on with that operator+ for the String class and toString (maybe related to operator precedence or something) - haven't looked too much into it yet. So there is a solution (use << instead of +) but I haven't thought too much why it's behaving that way...

@Saalvage
Copy link
Member Author

Making getTlsOssResult clean the stream was my first approach, however, it doesn't solve the issue when a stringification calls another stringification that depends on it, when the inner stringification is finished the whole stream is cleared.
This is why I built the stack-like solution in #569, is there anything wrong with it?

@onqtam
Copy link
Member

onqtam commented Jan 5, 2022

merged the PR #569

@onqtam onqtam closed this as completed Jan 5, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants