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

Compiling as Dll maybe is wrong #196

Closed
dimztimz opened this issue Mar 15, 2019 · 5 comments
Closed

Compiling as Dll maybe is wrong #196

dimztimz opened this issue Mar 15, 2019 · 5 comments

Comments

@dimztimz
Copy link
Contributor

I'll explain the correct scenario:

  1. We compile 1 dll.
  2. That dll contains more translation units.
  3. Only ONE TU compiles doctest, that is it includes fwd.h and the .cpp. The declarations in fwd must be marked as exported.
  4. The other TU may contain tests, they include only fwd.h, they only see the declarations. These declarations here also must be marked as exported. The bug lies here, doctest marks these declarations as imported.
  5. only the code that is OUTSIDE the dll will include fwd.h with the declarations marked as imported.

You should have two macros, or one with the values of 0, 1 and 2 that controls the dll attribute. DOCTEST_CONFIG_IMPLEMENT should not decide whether we are exporting or importing, as it does now.

@dimztimz
Copy link
Contributor Author

This issue is somewhat of lower priority as compiling doctest inside dll is rare.

@onqtam
Copy link
Member

onqtam commented Mar 15, 2019

But does the code work - is this just a warning? And is it a problem only on Windows (as I suspect)?

I suppose you are familiar with the DOCTEST_CONFIG_IMPLEMENTATION_IN_DLL option which is used in the multi-dll example. That option is necessary ONLY when you want to have a single test registry between multiple .dll (and maybe +1 .exe) files - in your case if you have tests only in a single .dll then you don't need that option and thus DOCTEST_INTERFACE should be defined to nothing.

An issue I remember - only on Windows! (as a warning which I think I silence) - is when DOCTEST_CONFIG_IMPLEMENTATION_IN_DLL is used - what ends up happening is that the TU in the .dll which implements the test runner "exports" the symbols using __declspec(dllexport) but all other TUs in the same .dll have forward-declarations of the same symbols with __declspec(dllimport). I think that warning is silenced and that it all works.

In order to be able to have a consistent __declspec(dllexport) annotation in all TUs of the exporting .dll and __declspec(dllimport) in all other .dll targets - then indeed I would need a second config option, but then again - the current example works...? I try to keep the use of the framework as simple as possible.

Also perhaps __declspec(dllimport) isn't even necessary so I can just remove it and still end up with a single config option? (and in the entire .dll which exports the test runner the annotations will be __declspec(dllexport))

So what is your exact workaround since it is of low priority? Is there a manifestation of a problem at all?

To be honest I want to push version 2.3 out with an xml reporter so people can use doctest properly on their CI in the next 10 days because after that I'll have perhaps the busiest 4 months of my life so my attention is on the xml reporter.

And in case you are really writing tests in multiple .dll files - that might make you the first person/organization I know of that uses this feature of doctest! (apart from me :D - I made it for myself)

And are you writing tests along with your production code - the spirit of the framework? Splitting tests into multiple .dll files is rarely necessary unless you are indeed writing them next to the actual code which is itself split into modules.

@dimztimz
Copy link
Contributor Author

the current example works...? I try to keep the use of the framework as simple as possible.

Well, "works for me" is not a real argument. The example works because the example itself is wrong and is doing overlinking. I discovered this bug when I tried to fix the ovelinking in the example in the cmake scripts.

Also perhaps __declspec(dllimport) isn't even necessary so I can just remove it and still end up with a single config option?

It is necessary.

To be honest I want to push version 2.3 out with an xml reporter so people can use doctest properly on their CI in the next 10 days because after that I'll have perhaps the busiest 4 months of my life so my attention is on the xml reporter.

Indeed this is a lower priority, I might make a patch some day in the future.

And are you writing tests along with your production code - the spirit of the framework?

No :). I discovered the bug when tried to fix the cmake scripts of the dll example.

@onqtam
Copy link
Member

onqtam commented Mar 15, 2019

Perhaps exploring if __declspec(dllimport) is even necessary is the best approach - on UNIX there is no such thing.

@dimztimz
Copy link
Contributor Author

On windows

When compiling the dll you must have dllexport.

When consuming the dll you must have dllimport. You can not skip this, you will get linking error.

On linux, you don't need anything in the declarations, unless you explicitly compile the dll with default visibility to be hidden. Then you need to prefix the declarations with "visibility default" only when compiling. When consuming it does not matter if you have visibility attributes or not, I think.

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