It is trivial to get rid of 2 of the 3 defines. Using:
#ifdef __CYGWIN__ #define GMX_CYGWIN #endif /* We currently don't support MingW. And ICC also defines it */ #ifdef _MSC_VER #define GMX_NATIVE_WINDOWS #endif
The question is what do we want to do with GMX_CXX11. Only trajectoryanalysis/analysismodule.h, gromacs/commandline/cmdlineprogramcontext.h, gromacs/analysisdata/modules/histogram.h, gromacs/options/abstractoption.h, and gromacs/onlinehelp/helptopicinterface.h are public headers which also use our unique_ptr. Does it make sense to only use shared_ptr for public API but keep using our unique_ptr internally?
Change some gmx_unique_ptr to shared_ptr in public headers
Change "trivial" occurrences of gmx_unique_ptr to boost::shared_ptr in
cases where there is a simple factory or a sink function where the
ownership of the object passes in or out from the library.
The main advantages:
- Removes GMX_CXX11 dependencies from installed headers.
- Improves memory management, where the memory is deallocated in the
same library/executable that allocated it (through the shared_ptr
deleter that gets created during construction).
The only disadvantage is that this blurs the ownership of the objects
There are only two other instances of gmx_unique_ptr left in installed
headers, and those should be relatively easy to remove as well with a
bit of refactoring.
Related to #1454.
Remove selection help from SelectionCollection
Instead, make the only caller outside the selection module to use
selhelp.h directly. This is not significantly messier, and allows
removing one use of gmx_unique_ptr from an installed header.
To actually remove that instance, do not install helptopicinterface.h.
This has the added benefit that now the whole onlinehelp module is free
from public headers, making the interface cleaner. Adjust doxygen
Part of #1454
Remove gmx_unique_ptr from options public headers
There was a single factory function in the generic option interface that
created objects and passed ownership to the caller. The implementation
for this method always follows the same pattern, and there only a single
caller, so it is easy to make it exception-safe while returning a plain
pointer. As a side effect, it removes some boilerplate code from all
the implementations for this method.
There are more complicated alternatives that avoid the plain pointer,
based on creating a helper class that can only be forward declared in
the installed headers, but I don't think those are worth the extra code.
This removes the last use of gmx_unique_ptr in an installed header.
Part of #1454
Move GMX_CXX11 to config.h
Now that uniqueptr.h is not used by any installed header, do not install
it. Move GMX_CXX11 that was used only by this file to config.h now that
config.h can be freely included in that file.
Part of #1454
Clean up gmx_header_config
- Remove some probably unnecessary uses for GMX_CYGWIN.
- Move GMX_CYGWIN to config.h as it is not used in installed headers
(maybe it would be better to get rid of even the remaining uses in
favor of some other mechanism).
- Remove gmx_header_config_gen.h and all complexity that supports it as
there is nothing generated there any longer.
- GMX_NATIVE_WINDOWS could possibly be moved somewhere else, but
currently it is only used in two headers, so possibly even better
would be to just get rid of those two.
Now we no longer install a header just to get platform
independence for functionality that is not actually
part of the the installed API.
Moved DIR_SEPARATOR into its own non-installed header.
Moved snprintf MSVC-workaround define into its own non-installed
Moved GMX_NATIVE_WINDOWS definition to config.h
Removed checks related to the use of gmx_header_config.h
#1 Updated by Teemu Murtola almost 6 years ago
- Category set to build system
It would be nice if you provided some justification for why you want to do this. I don't necessarily disagree, but just recently in https://gerrit.gromacs.org/#/c/3157/ you seemed to propose exactly the opposite, tying the headers much more tightly to one instance of the library. Also, what would be the plan to deal with other config.h dependencies in installed headers? Currently, they are in a much better shape than what they used to be, though, with majority of the issues coming from thread_mpi. It should not be very difficult to get rid of GMX_CYGWIN and GMX_NATIVE_WINDOWS in installed headers completely, either, which would be even better.
For the public API, I think it would make a lot of sense to only use shared_ptr to remove that dependency of the public API on the internal configuration options. That may ripple through to a lot of the internal implementation, though, since it is not possible to convert between the pointer types. If it ends up removing a large fraction of current unique_ptr instances, the question then is, is it worth to keep the unique_ptr at all, with all the complexity it brings, or just replace it with shared_ptr everywhere? Or alternatively, should we use raw pointers at some parts to avoid these smart pointers on the API? But I won't object to such a change, just wanted to bring up these points, since you were quite aggressively pushing for use of smart pointers in #887.
#2 Updated by Roland Schulz almost 6 years ago
While working on #1452 I considered the TODO you added to gmx_header_config_gen.h and noticed that part of the problem is trivial. So the reason is mostly because of the TODO you put there ;-). But I agree with your TODO. I think if we want a API the API should ideally not depend on any ifdef.
I opened this task to ask what we wanted to do with the 3rd. You are right that I don't know what the best approach is, and thus I suggested different solutions in the past and here.
I think tightly coupling the header to the library is an option if we really have to make the API different. And as long as e.g. only GMX_DOUBLE is left than we could do it nicely the same as fftw (having both fftw3 and fftw3f). But I think we agree that it would be helpful if the number of variables with change the API is limited as much as possible.
I don't see a big advantage of getting rid of variables like GMX_CYGWIN or GMX_NATIVE_WINDOWS if there don't come from cmake but from some preprocessor logic as suggested in the question. Especially if they don't prevent compiling libgromacs with one compiler and using it with a different one. What is the reason you would like to remove them altogether?
shared_ptr has a constructor for unique_ptr thus that direction is no problem. But the other direction isn't possible. But even with just being able to convert it in one direction it might help that it doesn't ripple as far. The question is how far would it ripple. But it probably still ripples too far to make this a good idea. I think there is an advantage of keeping unique_ptr which is bigger than getting rid of GMX_CXX11. So my idea was probably not good.
I think having thought about it more I think replacing GMX_CYGWIN and GMX_NATIVE_WINDOWS as shown above and tightly coupling for GMX_DOUBLE and GMX_CXX11 is the best solution. What do you think?
#9 Updated by Teemu Murtola over 5 years ago
- Status changed from Fix uploaded to Resolved
- Assignee set to Teemu Murtola
Now there's a single define left in gmx_header_config.h, and the generated header is gone. It should be straightforward to get rid of this as well in installed headers and move to config.h, but that's outside the scope of this issue.