Skip to content
This repository has been archived by the owner on Jun 24, 2021. It is now read-only.

Full C++14 integration. #198

Merged
merged 23 commits into from
Jul 23, 2016
Merged

Full C++14 integration. #198

merged 23 commits into from
Jul 23, 2016

Conversation

jevolk
Copy link
Contributor

@jevolk jevolk commented Jun 27, 2016

This allows the option of at least -std=gnu++14 for new translation units.
Changes are trivial.

@jevolk jevolk changed the title Fix compatibility with c++ dialects. Full C++14 integration. Jun 29, 2016
@grawity
Copy link
Contributor

grawity commented Jul 16, 2016

no and ircd: 😱 are not acceptable commit messages. Even if it seems painfully obvious to the author, please still explain why things were removed for the benefit of everybody else reading these commits.

(I'm also really not sure Boost has a place in charybdis, but oh well.)

@jevolk
Copy link
Contributor Author

jevolk commented Jul 16, 2016

@grawity I've merely censored the real commit message to protect the children. 65,000 calls to close(), 90's-era daemonization, and screwing with the implicit fd's is sub-optimal.

P.S. looking now, there's a fair amount here that will be filtered out of the first merge anyway...

@dwfreed
Copy link
Contributor

dwfreed commented Jul 16, 2016

At first I thought this was a serious pull request, and then I did some actual looking at it, and now I'm not sure anymore. Let's take a look at some of the issues I've seen in my few minutes looking at it:

  • Boost? We now need boost to build an ircd that has only depended on very basic system libraries since its inception?
  • Even worse, bundling boost? I realize boost is a piece of shit and breaks things all the time, but your solution is to basically open everybody up to security vulnerabilities out of the box? There's a reason Linux distros have rules basically forbidding bundling wherever possible: trying to keep track of security vulnerabilities in 50 versions of a library is an absolute nightmare, when there's a system copy right over there that probably works (except maybe in boost's case, in which case that's what patches are for). Also, a boost build takes ~100 MB on disk; that's 100 MB people don't need twice.
  • I love it when starting my ircd prints a banner that doesn't even fit on my screen
  • I love it even more when my compiler only tells me about at most 3 errors at a time, so I have to fix those errors, try to build again, and repeat until I've finally fixed all the problems, rather than getting all the current errors at once, so I can fix them all at once, and then only have to worry about actually new errors.

All this because you think C++ is absolutely a superior language to C, and believe that magic is the answer to everything. I'm sorry, but I prefer being able to sanely debug what my ircd is doing. If this is the direction charybdis is going, I guess I will need to find another ircd at some point.

@kaniini
Copy link
Contributor

kaniini commented Jul 16, 2016

@dwfreed my understanding is that we're only building the parts of boost that we are actually using (essentially boost.asio), and the submodule inclusion is a better approach than just importing a snapshot. building only ASIO is not very involved, and is of higher quality than something we would reinvent ourselves. would forking boost.asio to avoid pulling in the submodule really be a better solution?

@kaniini
Copy link
Contributor

kaniini commented Jul 16, 2016

fwiw i am pretty sure that banner was just a joke, and isn't going to be included in the final release of charybdis 5.

@dwfreed
Copy link
Contributor

dwfreed commented Jul 16, 2016

@kaniini just use system boost if you're going to use boost; 99% of people who run charybdis do it on a binary distro anyway, and they probably already have boost because something else uses it.

@kamilion
Copy link

ISO C11 support I could understand. building as C++, I cannot.
This patchset worries me a lot; I've been running charybdis for five years, and now suddenly I have to worry about objects leaking memory, building boost or installing out of date boost 1.46/1.48 packages that don't even seem to include libboost-asio... What is the projected gain from this patchset? Did I somehow miss a github issue where this was a requested feature? Are people actually interested, or is this just a bunch of try-it-and-see work? The dates on the commits only stretch back a month, so how well debugged is this code with the build environment altered so much?

@charybdis-ircd charybdis-ircd locked and limited conversation to collaborators Jul 16, 2016
@kaniini
Copy link
Contributor

kaniini commented Jul 16, 2016

I'm restricting this patchset to contributors only, as I really don't want to hear whining. Use the software or don't. Thanks for your understanding.

@jevolk
Copy link
Contributor Author

jevolk commented Jul 16, 2016

@dwfreed

Boost? We now need boost to build an ircd that has only depended on very basic system libraries since its inception?

Yes, the ratbox event system is really not good; contrast to libmowgli for example. It's inadvisable to waste effort improving the ratbox event system- buggy code ripped from squid decades ago which doesn't take advantage of modern OS features in a reliable cross-platform way. Boost allows for optimal and evolving cross-platform support from a single IO interface. This is what you want.

  • Optimal cross-platform support: Experts for each specific platform figure out the best way to implement the interface.
  • Evolving cross-platform support: New and better solutions are contributed as the technology becomes available in the platform -- we change and maintain nothing.

Even worse, bundling boost?

We don't build boost. Calm down. Boost is mostly header only libraries. A few of the big ones are linked. Right now only boost_system is "built" and I'm looking into what is being linked to... something like errno, and I'm pretty sure from past experience it will be compatible with the shared library probably mapped into your ram somewhere as we speak...

open everybody up to security vulnerabilities out of the box? There's a reason Linux distros have rules basically forbidding bundling wherever possible: trying to keep track of security vulnerabilities in 50 versions of a library is an absolute nightmare

Please no need to fear-monger. I have no idea what you're talking about. I want to pin to a specific version of boost so I know what's being compiled in, and that's what is being done here with submodules. If distros have a problem with recursive git trees and submodules as, quite possibly, a new paradigm of packaging distros themselves, maybe they should talk to me.

I love it when starting my ircd prints a banner that doesn't even fit on my screen

It's a tornado that says tulsa oklahoma somewhere in it. relax.

I love it even more when my compiler only tells me about at most 3 errors at a time, so I have to fix those errors, try to build again, and repeat until I've finally fixed all the problems, rather than getting all the current errors at once, so I can fix them all at once, and then only have to worry about actually new errors.

Well then you're not writing C++ 👯

@jevolk
Copy link
Contributor Author

jevolk commented Jul 16, 2016

Thanks for all the support. Visit irc.charybdis.io #charybdis

jevolk added 3 commits July 19, 2016 22:38
This allows the option of at least -std=gnu++14 for new translation units.
Changes are trivial.
Files with the suffix .cpp will automatically integrate.
@charybdis-ircd charybdis-ircd unlocked this conversation Jul 20, 2016
@kaniini
Copy link
Contributor

kaniini commented Jul 20, 2016

Conversation on the patchset itself is reopened. Please understand that:

  • this is not the final patchset
  • future versions of charybdis are 100% likely to leverage C++ as we can do more compile-time verification of correctness in the codebase
  • we plan to leverage boost.asio, but we want to use a known-good version of the boost libraries we use, so we will build a local copy of those libraries (not the entirety of boost itself)

Conversation should remain on topic -- advocating that we do not use C++ or certain boost modules -- is considered off topic and will result in the conversation being restricted again.

Thanks to everyone for their understanding.

* argument number of the format string, and b is the 1 based argument
* number of the variadic ... */
#ifdef __GNUC__
#define AFP(a,b) __attribute__((format (printf, a, b)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RB_AFP()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure but that just adds to this diff because the calls are all over... There will have to be a complete macro/define refactor next.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@kaniini
Copy link
Contributor

kaniini commented Jul 20, 2016

"Ratbox 2: HyperRatbox" is a silly commit description, please revise the commit descriptions around them as they are a little vague.

@kaniini
Copy link
Contributor

kaniini commented Jul 20, 2016

In general, no objections to the mechanical side of the patchset here. I think if we can squash it down a bit and get the build bots green, then we can merge it into 5.

@jevolk
Copy link
Contributor Author

jevolk commented Jul 20, 2016

Should be done tomorrow if the hangover is light.

@dwfreed
Copy link
Contributor

dwfreed commented Jul 20, 2016

  • I love it even more when my compiler only tells me about at most 3 errors at a time, so I have to fix those errors, try to build again, and repeat until I've finally fixed all the problems, rather than getting all the current errors at once, so I can fix them all at once, and then only have to worry about actually new errors.

When I wrote this, there was an AM_CPPFLAGS = -fmax-errors=3 in Makefile.am, which causes the compiler to exit after it's encountered 3 errors, hence this bullet point. That appears to have been removed since.

  • we plan to leverage boost.asio, but we want to use a known-good version of the boost libraries we use, so we will build a local copy of those libraries (not the entirety of boost itself)

@kaniini just know that distros (especially Debian and Gentoo) will maintain patchsets unbundling boost, and forcing charybdis to use the system copy, which may be a different version than what you're bundling, and thus have different bugs.

@dwfreed
Copy link
Contributor

dwfreed commented Jul 20, 2016

2 things:

  1. Since there is some (albeit small) compilation going on, you should not be building boost in autogen.sh, as different compilers will produce different results (with potentially incompatible ABIs), and ideally release tarballs will be post-autogen.sh.
  2. New headers should use #pragma once instead of old-fashioned include guards; this is supported on all compilers charybdis will be built on, lets the preprocessor decide how it wants to handle the issue, avoids issues with somebody accidentally putting a definition outside the guard, and avoids macro name collisions and having a crapload of useless macros.

@kaniini
Copy link
Contributor

kaniini commented Jul 20, 2016

We will probably support --with-system-boost as an option anyway, but distribution builds have never been considered "official" by us, and oddly enough, bugs reported in those packages tend to disappear when the end user rebuilds it themselves.

@jevolk
Copy link
Contributor Author

jevolk commented Jul 20, 2016

@dwfreed First realize this project has almost none of the working modern infrastructure that you probably take for granted in something like Atheme.

@kaniini just know that distros (especially Debian and Gentoo) will maintain patchsets unbundling boost, and forcing charybdis to use the system copy, which may be a different version than what you're bundling, and thus have different bugs.

This project already has precedent for bundling stuff, namely libltdl. I believe the default is to use system ltdl first and build the bundle optionally and I never assumed boost would work any different from the ltdl schema. I agree that --with-system-boost should actually be on and defaulting to the distro version first. Boost is robust enough and our usage of it is so light it really doesn't matter in terms of features and system boost's version on the targeted platform is nearly guaranteed to be sufficient within the last ~5 years.

Since there is some (albeit small) compilation going on, you should not be building boost in autogen.sh, as different compilers will produce different results (with potentially incompatible ABIs), and ideally release tarballs will be post-autogen.sh.

Boost is evalulated by the same compiler toolchain that the rest of the project uses. It builds with all of the same target platforms, GCC4,5,6, clang3, mingw, etc... With that being said, once there's sufficient detection for system boost availability in the libpath that step may rarely be seen anyway...

New headers should use #pragma once instead of old-fashioned include guards; this is supported on all compilers charybdis will be built on, lets the preprocessor decide how it wants to handle the issue, avoids issues with somebody accidentally putting a definition outside the guard, and avoids macro name collisions and having a crapload of useless macros.

Also on this point, the #ifdefs have the same optimization that #pragma once has and the added benefit of being able to check in a later #ifdef for whether something was included or not and decide more things on it... All of these #ifdef tokens have to be cleaned up with a uniform prefix and syntax like _RB_SOMETHING_H or HAVE_RB_SOMETHING_H etc. @dwfreed I will accept any pull requests directly to this branch regarding any of that getting out of the way.

P.P.S If the #pragma once schema has a way to detect if something was or wasn't included then it is definitely superior now.

@dwfreed
Copy link
Contributor

dwfreed commented Jul 20, 2016

Boost is evalulated by the same compiler toolchain that the rest of the project uses. It builds with all of the same target platforms, GCC4,5,6, clang3, mingw, etc...

Eh? My point is that compilers can and sometimes do change ABI between versions. If you build boost with GCC 6.whatever, and ship that in the release tarball, and then I go and build charybdis with GCC 4.9.3, which hypothetically ends up having incompatible ABI, then the build is broken. Just make building boost a very early target in the makefile, and skip it in the --with-system-boost case.

@jevolk
Copy link
Contributor Author

jevolk commented Jul 20, 2016

Yes, a target in the makefile sounds better.
On Jul 20, 2016 8:49 AM, "Doug Freed" [email protected] wrote:

Boost is evalulated by the same compiler toolchain that the rest of the
project uses. It builds with all of the same target platforms, GCC4,5,6,
clang3, mingw, etc...

Eh? My point is that compilers can and sometimes do change ABI between
versions. If you build boost with GCC 6.whatever, and ship that in the
release tarball, and then I go and build charybdis with GCC 4.9.3, which
hypothetically ends up having incompatible ABI, then the build is broken.
Just make building boost a very early target in the makefile, and skip it
in the --with-system-boost case.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#198 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEdz0jnkwsYGmPOITthJhav-pTuGBxD1ks5qXkN_gaJpZM4I-5nV
.

@jevolk
Copy link
Contributor Author

jevolk commented Jul 20, 2016

@kaniini @dwfreed Since these all have to be redone project-wide, how do you feel about an include schema like:

#pragma once
#define HAVE_IRCD_CLIENT_H

This elides the #endif at the bottom.

Note: I'm accepting pull requests straight on to this pull request :)

jevolk added 20 commits July 21, 2016 20:51
* librb is no longer a separately configured subproject.
* charybdis is now a standalone directory with a binary.
* Include path layout now requires a directory ircd/ rb/ etc.
Adds a Makefile target 'mrproper' though this only works on new versions of AC.
ircd/rb: Move some lowish level macros down to rb.
Happy 28th birthday. You're all grown up.
NOTE: This only has "C" linkage right now.
This test against the __cplusplus #define value allows for a more
granular feature test.
If you feel the need to peep around.
@kaniini kaniini merged commit 8fed90b into charybdis-ircd:master Jul 23, 2016
@jevolk jevolk deleted the cpp branch July 23, 2016 21:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants