Skip to content
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

Reduce scope of DebugOutputWindowReporter instance #174

Closed
bnason-nf opened this issue Jan 23, 2019 · 4 comments
Closed

Reduce scope of DebugOutputWindowReporter instance #174

bnason-nf opened this issue Jan 23, 2019 · 4 comments

Comments

@bnason-nf
Copy link
Contributor

bnason-nf commented Jan 23, 2019

Hi @onqtam,

Using doctest on Windows, I was seeing an issue with the order of global static constructors, causing the output of the DebugOutputWindowReporter to be ignored. This patch fixes that:

index c8d3d6a..80f52cc 100644
--- a/doctest/parts/doctest_impl.h
+++ b/doctest/parts/doctest_impl.h
@@ -2076,8 +2076,6 @@ namespace {
     };

     DOCTEST_THREAD_LOCAL std::ostringstream DebugOutputWindowReporter::oss;
-
-    DebugOutputWindowReporter g_debug_output_rep;
 #endif // DOCTEST_PLATFORM_WINDOWS

     // the implementation of parseFlag()
@@ -2400,6 +2398,7 @@ int Context::run() {

     // always use the debug output window reporter
 #ifdef DOCTEST_PLATFORM_WINDOWS
+    DebugOutputWindowReporter g_debug_output_rep;
     if(isDebuggerActive())
         p->reporters_currently_used.push_back(&g_debug_output_rep);
 #endif // DOCTEST_PLATFORM_WINDOWS

If you like, I can create a pull request for this.

Thanks,
Benbuck

@onqtam
Copy link
Member

onqtam commented Jan 23, 2019

Thats weird - I cannot imagine what the problem could be. But it wouldn't hurt to incorporate this change - the other reporter (the console one) has its scope reduced as well anyway. I'll push a fix in the dev branch.

onqtam added a commit that referenced this issue Jan 23, 2019
@onqtam
Copy link
Member

onqtam commented Jan 23, 2019

Just pushed a "fix" in the dev branch - not sure when I'll release 2.2.2 but if it is important to be in an official release I'll do it. But I still cannot explain why this would be a problem - there is nothing special happening when reporters are constructed and they aren't used until main() is entered - so it's still a mystery to me. I guess it's always better to have less globals! :)

@bnason-nf
Copy link
Contributor Author

bnason-nf commented Jan 24, 2019

Hi @onqtam,

Thanks for pushing this.

The issue I'm seeing when g_debug_output_rep is at global scope is that when the constructor runs, the oss thread local static variable is uninitialized. I don't understand why that is though. If I change oss to not be thread local, then it is initialized, so maybe the thread local variables are being initialized to late in this case?

-Benbuck

@onqtam
Copy link
Member

onqtam commented Jan 24, 2019

@bnason-nf weird... there are other thread_local globals and apparently it works OK with them... :D
Anyway this change doesn't hurt.
I'll push it in master when I release the next version - thanks for reporting!

@onqtam onqtam closed this as completed in fa85667 Jan 25, 2019
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

No branches or pull requests

2 participants