DOCTEST_CONFIG_USE_STD_HEADERS in MSVC++

[Cvelth]

It seems there's no need to include whole <sstream> header. Just <ostream> is enough.

[TheLartians]

Hey thanks for the PR! Good to reduce the problem a little. Do you happen to know why this is required though? Also it seems that GitHub actions aren't running on your fork so I cannot verify the fix. Could you enable them for your repository?

Edit: I've enabled local actions to run on PRs as well. If you pull from the master branch the actions should trigger as well.

[codecov[bot]]

Codecov Report

Merging #30 into master will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##            master       #30   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           12        12           
=========================================
  Hits            12        12           

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 716a30d...27bdaa3. Read the comment docs.

[Cvelth]

I'm on to a different solution See doctest docs for information.

After all, defined by the doctest macro seems like a better solution.

EDIT: link fix.

[TheLartians]

Hey thanks for finding the option, now the error makes more sense!

In that case I think we should remove the local macro and just enable the flag by default for MSVC to avoid conflicting definitions and problems in other test files.

I think it should work if we add the following to the tests/CMakeLists.txt:

  elseif(MSVC)
    target_compile_options(Greeter PUBLIC /W4 /WX)
++  target_compile_definitions(GreeterTests PUBLIC DOCTEST_CONFIG_USE_STD_HEADERS)
  endif()

[Cvelth]

I totally agree. But we should somehow be explicit about defining this macro, as it does slightly change doctest's behavior. When it's defined, overloaded by the doctest operator<<'s could run into some problems. Testing is required to know definitely, though.

EDIT: But yes, doctest documentation mentions

This should be defined globally.

as well.

[TheLartians]

Also looking at the source code it seems that it's only creating some forward declarations of std types, so as long as the code compiled before we should still be ok after defining the macro.

[Cvelth]

Ok, I see your point. So which's better, to create new pull request (branch) for the fix or to just add new commit here?

[TheLartians]

Just simply add the commit here, then the full discussion will be visible. I'll squash all commits before the merge so the main git's history will remain clean.

[TheLartians]

Looks good, thanks for the fix!