Emil Dotchevski wrote:

I am against such a move. Boost Threads requires linking for other features which makes it one of the few libraries in Boost that can be properly designed to avoid unnecessary physical coupling. Unless something is proven to cause performance problems it should not be inlined, regardless of how simple it is.

I don't see your point. AFAIK, mutexes do not require any features that have to reside in a separately compiled library. And forcing them to depend on it discourages from using Boost.Thread in header-only libraries (those in Boost and outside of it) and justifies things like lightweight_mutex.

Emil Dotchevski wrote:

...

...

I am against such a move. Boost Threads requires linking for other features which makes it one of the few libraries in Boost that can be properly designed to avoid unnecessary physical coupling. Unless something is proven to cause performance problems it should not be inlined, regardless of how simple it is. I don't see your point. AFAIK, mutexes do not require any features that have to reside in a separately compiled library.

In principle, nothing ever has to reside in a separately compiled library.

That's an exaggeration, just as having everything separately compiled.

You don't see my point because you think of the header-only approach as a good thing,

Not exactly. There are pros and cons, and in this case I find pros more valuable. And there are other header-only Boost libraries that I would rather have separately compiled.

whereas in my mind headers should be limited to things that must be in a header, such as functions for which inlining is critical, as well as template definitions. This reduces physical coupling, which is only a problem in large scale projects; unfortunately by the time it becomes a problem it is already too late.

I think, this particular case doesn't increase coupling in any considerable way. It won't introduce any header includes, classes or new functions, AFAICT. Besides, the need to link to the library is a way of coupling, too, so we'll remove it with the change.

On Wed, Apr 8, 2009 at 2:34 AM, Anthony Williams <anthony.ajw@gmail.com> wrote:

Emil Dotchevski <emildotchevski@gmail.com> writes:

...

By the way, Boost is way past that point, so a good argument against my position is that one more header-only lib isn't going to make things (much) worse. :)

And in this case, boost::mutex is mostly header-only already, so it's only a few empty inline functions for the exceptions that will be moving.

Yes, that was my point, but I still wouldn't do it. An empty non-inline function can be very useful debugging tool. For example, defining a non-inline constructor for an exception type allows you to temporarily modify the constructor to print diagnostic information whenever someone throws that exception -- at the cost of re-compiling a single cpp file. For very large projects this can be an incredible win. Of course, this is just an example, my general stance is that you can't predict all the benefits of minimizing code in header files (which is the most important tool for keeping physical coupling under control.) Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode

Anthony Williams wrote:

In any case, after further thought I remembered the reason I didn't do this before. If the exceptions are header-only then they cannot be thrown from a DLL and caught outside the DLL, since the type-ids won't match. This would mean that any exceptions thrown by the DLL version of boost.thread wouldn't be able to be caught in user code other than by catching std::exception or with catch(...). This is the case for MSVC/Windows anyway --- I'm not sure about other compilers/platforms.

Is it so? On which MSVC version? I'm asking because I throw/catch exceptions across dll boundaries all the time on Windows and haven't had any problems yet. There are some issues on Linux, but they are solved (more or less) with visibility tunings.

Quoting Anthony Williams <anthony.ajw@gmail.com>:

Andrey Semashev <andrey.semashev@gmail.com> writes:

...

Anthony Williams wrote:

...

In any case, after further thought I remembered the reason I didn't do this before. If the exceptions are header-only then they cannot be thrown from a DLL and caught outside the DLL, since the type-ids won't match. This would mean that any exceptions thrown by the DLL version of boost.thread wouldn't be able to be caught in user code other than by catching std::exception or with catch(...). This is the case for MSVC/Windows anyway --- I'm not sure about other compilers/platforms.

Is it so? On which MSVC version? I'm asking because I throw/catch exceptions across dll boundaries all the time on Windows and haven't had any problems yet. There are some issues on Linux, but they are solved (more or less) with visibility tunings.

Hmm. I'll have to try it out when I get a chance. Maybe I'm mistaken, but that's certainly my recollection.

If everything does work then that hurdle is removed.

Well I do that within my own code all the time too so I know it works on MSVC. But I am in complete control of that code. Item 62 from Sutter and Alexandrescu, "Don't allow exceptions to propagate across module boundaries", is germane. Actually their advice is more specific than the title suggests: "Don't allow exceptions to propagate between two pieces of code unless you control the compiler and compiler options used to build both sides".

Wow, that's a hard line you've drawn there. I'm not sure I agree. The only reason this matters is if you're going to be changing the implementation and don't want to recompile the code that uses the header. For users compilation time matters as well. Users who don't use

Anthony Williams wrote: precompiled headers have to recompile a header only library over and over.

For boost users, the implementation only changes if they change boost versions, and in that case I would expect people to recompile anyway --- I wouldn't trust something compiled against boost 1.37 to link against a 1.38 lib, for example.

In any case, after further thought I remembered the reason I didn't do this before. If the exceptions are header-only then they cannot be thrown from a DLL and caught outside the DLL, since the type-ids won't match. This would mean that any exceptions thrown by the DLL version of boost.thread wouldn't be able to be caught in user code other than by catching std::exception or with catch(...). This is the case for MSVC/Windows anyway --- I'm not sure about other compilers/platforms.

I could change the code so that if you're linking against the static library then the exception functions are inline, since they can't be thrown across DLL boundaries anyway in this case. That would mean that if you were static linking against boost.thread then you wouldn't actually need the lib in some cases (e.g. if all you used was boost::mutex).

Anthony

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 09 April 2009, Anteru wrote:

Dmitry Goncharov schrieb:

I tend to agree here -- I had to wrap all thread usage behind a PIMPL because previously it would include <windows.h>, which is deadly for compile times.

Unless there are very compelling reasons to move the stuff into the header (like, all other mutex implementations in Boost get removed), then I can understand it, but otherwise I'd leave it as it is. The thing is, the argument that lightweight_mutex /could/ be removed is bogus until there is some definite plan to remove it while doing this change, otherwise we'll end up with having both to pay the price of higher compile times in Boost.Thread and having a mostly redundant class somewhere else.

Isn't arguing that boost::mutex shouldn't be made header-only due to concerns about compile time even more bogus? The reason the header-only suggestion was brought up in the first place was that the code in question is so trivial it won't impact compile times to put in entirely in the header. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) iEYEARECAAYFAkneEZUACgkQ5vihyNWuA4X/pQCgtHso+9ILb7HWSUkAqIt2JvP0 uN4AoKlXJRDfpiA5PI8UYBihL1R52kQM =xEx4 -----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 09 April 2009, Dmitry Goncharov wrote:

Frank Mori Hess wrote:

...

Isn't arguing that boost::mutex shouldn't be made header-only due to concerns about compile time even more bogus? The reason the header-only suggestion was brought up in the first place was that the code in question is so trivial it won't impact compile times to put in entirely in the header.

The current version of boost/thread/pthread/mutex.hpp directly includes 11 headers.

And your point is? I'm not saying including mutex.hpp has no effect on compile time. I'm saying making the changes that would free mutex.hpp from dependence on the compiled boost.thread library would have no effect on compile time (as compared to the current version). -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) iEYEARECAAYFAkneFzgACgkQ5vihyNWuA4XNuwCaA39VGu4fhjdrBf11rCHRZsG/ vLcAoI48bloHsUX/V69PzlI8Ff7VuL5A =SQDv -----END PGP SIGNATURE-----

On Thu, Apr 9, 2009 at 8:17 AM, Frank Mori Hess <frank.hess@nist.gov> wrote:

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

On Thursday 09 April 2009, Anteru wrote:

...

Dmitry Goncharov schrieb:

I tend to agree here -- I had to wrap all thread usage behind a PIMPL because previously it would include <windows.h>, which is deadly for compile times.

Unless there are very compelling reasons to move the stuff into the header (like, all other mutex implementations in Boost get removed), then I can understand it, but otherwise I'd leave it as it is. The thing is, the argument that lightweight_mutex /could/ be removed is bogus until there is some definite plan to remove it while doing this change, otherwise we'll end up with having both to pay the price of higher compile times in Boost.Thread and having a mostly redundant class somewhere else.

Isn't arguing that boost::mutex shouldn't be made header-only due to concerns about compile time even more bogus?  The reason the header-only suggestion was brought up in the first place was that the code in question is so trivial it won't impact compile times to put in entirely in the header.

Compile times aren't the issue at all, coupling is. For example, using precompiled headers improves compile times but increases coupling. The benefit of minimizing code in headers is that it increases the chance that updating the build after changing the source code will be limited to compiling a single CPP file, as opposed to any number (up to thousands for large projects) of CPP files when headers are modified. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode

On Tue, Apr 7, 2009 at 1:55 PM, Anthony Williams <anthony.ajw@gmail.com> wrote:

Emil Dotchevski <emildotchevski@gmail.com> writes:

...

On Tue, Apr 7, 2009 at 8:57 AM, Anthony Williams <anthony.ajw@gmail.com> wrote:

...

joaquin@tid.es writes:

...

Looks like a sensible part of Boost.Threads, namely that dealing with mutexes and locks, would be header-only but for its relying on boost/thread/exceptions.hpp, whose implementation is located at /libs/thread/*/exceptions.cpp.

Given that this .cpp mostly consists of extremely simple definitions (as simple as do-nothing functions in many cases), would it make sense to move this to inline definitions in boost/thread/exceptions.hpp thus making mutexes and locks header-only? This would greatly enhance the appeal of this part of Boost.Threads, as having to link a separate module is a considerable burden.

That seems sensible.

I am against such a move. Boost Threads requires linking for other features which makes it one of the few libraries in Boost that can be properly designed to avoid unnecessary physical coupling. Unless something is proven to cause performance problems it should not be inlined, regardless of how simple it is.

Wow, that's a hard line you've drawn there. I'm not sure I agree. The only reason this matters is if you're going to be changing the implementation and don't want to recompile the code that uses the header. For boost users, the implementation only changes if they change boost versions, and in that case I would expect people to recompile anyway --- I wouldn't trust something compiled against boost 1.37 to link against a 1.38 lib, for example.

You're arguing that physical coupling to something like Boost isn't a problem. I'm arguing that introducing any physical coupling must be justified. From certain point of view both arguments are valid, which is why we disagree. :)

In any case, after further thought I remembered the reason I didn't do this before. If the exceptions are header-only then they cannot be thrown from a DLL and caught outside the DLL, since the type-ids won't match. This would mean that any exceptions thrown by the DLL version of boost.thread wouldn't be able to be caught in user code other than by catching std::exception or with catch(...). This is the case for MSVC/Windows anyway --- I'm not sure about other compilers/platforms.

I'm not sure I understand why the type IDs wont match, but I do believe you.

I could change the code so that if you're linking against the static library then the exception functions are inline, since they can't be thrown across DLL boundaries anyway in this case. That would mean that if you were static linking against boost.thread then you wouldn't actually need the lib in some cases (e.g. if all you used was boost::mutex).

In my book this is another example of unnecessary physical coupling. If I'm calling Boost Thread from an exception-neutral context, I'd rather not see definitions of exception types (not that exceptions are in any way special, this is just an example.) Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode