From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id C22153858001 for ; Fri, 4 Dec 2020 17:08:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C22153858001 Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-215-cTSrk1tmONa3E0nAeWDr8w-1; Fri, 04 Dec 2020 12:08:30 -0500 X-MC-Unique: cTSrk1tmONa3E0nAeWDr8w-1 Received: by mail-qk1-f199.google.com with SMTP id 141so5728218qkh.18 for ; Fri, 04 Dec 2020 09:08:30 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=NFACTX9rnz0VEKEoupiTWRXByFzuKKov5Ptjp65E+h0=; b=tUNexdDx/IXMNTBzk/ez5UrbWFuOs0u1/UzRc0ltbpHsm1UQlwU9LBBEFdky1zCuXU 3ZSUs5ex1XttkxDeKkylPbpSjp3G7iWUCuicEJbdWpr+X0PuNviQbKFiiXGgvAwHi56/ B++IYLXCpeE8C2g25fuAyrchRacnRx4tJ81/v3sZIOMdL8ZOtJjaQVmC337f9ZgdgG/C 0orl2T/rGCtsXJ3ManeieoSRbxU8SOUi81LIy99J1e88vTYL4Ln3spLrR7pESBIQ3Xs9 0soQwb64nUY8vuKQcnDznGo0z9KyurtErC8VLG3NLOkQqG+OPqUZN/WrxncSxpgtAKNd 9e5A== X-Gm-Message-State: AOAM532W17P1/jhbI8i5b/Bp+3KtOf4z2K2DflwYFJf94D918xPr1veh MOiT7yx7xIJ9P7F/Mpq7ZYpAWXTYV93LKDJhTJbumEIzTm8Z4a2SWL7oQH2m/ILveHIzBeScFSZ uJDW7UjcZXVXvZupcY9fw X-Received: by 2002:ac8:2a45:: with SMTP id l5mr10021406qtl.153.1607101709464; Fri, 04 Dec 2020 09:08:29 -0800 (PST) X-Google-Smtp-Source: ABdhPJyEK8uD4y3EgS/V3n/LREzv+gqfrcUzbmB5Nd32sqBNwgo+PAkdQr7pJH4LJ2yF9Fhidba7wQ== X-Received: by 2002:ac8:2a45:: with SMTP id l5mr10021364qtl.153.1607101708852; Fri, 04 Dec 2020 09:08:28 -0800 (PST) Received: from [192.168.1.243] (47-208-193-143.trckcmtc01.res.dyn.suddenlink.net. [47.208.193.143]) by smtp.gmail.com with ESMTPSA id o22sm6106331qto.96.2020.12.04.09.08.18 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 04 Dec 2020 09:08:23 -0800 (PST) Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.20.0.2.21\)) Subject: Re: [PATCH] Use C++11 for the code base From: Ben Woodard In-Reply-To: <86ft4nowan.fsf@redhat.com> Date: Fri, 4 Dec 2020 09:08:16 -0800 Cc: libabigail@sourceware.org Message-Id: <1592EE35-F839-42A9-82A0-B60D4B537656@redhat.com> References: <86ft4nowan.fsf@redhat.com> To: Dodji Seketeli X-Mailer: Apple Mail (2.3654.20.0.2.21) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Dec 2020 17:08:38 -0000 > On Dec 3, 2020, at 5:26 AM, Dodji Seketeli via Libabigail wrote: >=20 > Hello, >=20 > As the Enterprise Linux 6 platform has now essentially reached its > end of life for what it's worth (the Fedora EPEL6 distribution is not > maintained anymore) nothing ties us to using C++03 only anymore. >=20 > So, I think it makes sense to move the code base to the C++11 > standard. >=20 > Why C++11 and not, say, C++14 or more? Well, the more direct reason I > see is that we need to support long life cycle platforms, the older > one being Enterprise Linux 7 currently. This is the Fedora EPEL7 > distribution, in concrete terms. And in that distribution, the > compiler is GCC 4.8.x. And it supports C++11. Why don=E2=80=99t you move the =E2=80=94enable-cxx11 forward to =E2=80=94en= able-cxx17 then we can make sure that the codebase has no problems moving f= orward to that level of the language. In particular I think that c++14 fixe= s some issues with c++11 and even though RHEL7 can=E2=80=99t handle it out = of the box, it would be beneficial to move in that direction. 17 and 20 are= more just to make sure that the code base is ready to be compiled with tho= se stricter standards. >=20 > In practise, nothing changes in the code that is already there. >=20 > The new code however can use C++11 constructs just fine. >=20 > I have updated the CONTRIBUTING file to write down some of the unwritten > cultural biases of the current code base. Hopefully these few lines > will help to shed some light on the choices made so far. >=20 > The update to that file also enacts the use of C++11 and sets some > limits to what we expects in terms of what the code base would look > like. >=20 > configure.ac is modified to unconditionally pass -std=3Dc++11 to the > compiler and express that in the configuration text displayed at the > end of the configuration stage. >=20 > Some Makefile.am files are updated accordingly. >=20 > =09* CONTRIBUTING: Enact use of c++11. Also, we favor those who > =09read/debug/maintain the code as opposed to those who write it ;-) > =09* configure.ac: Switch to c++11 unconditionally. > =09* src/Makefile.am: Adjust. > =09* tests/Makefile.am: Adjust. >=20 > Signed-off-by: Dodji Seketeli >=20 > Applied to master. >=20 > --- > CONTRIBUTING | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++= +++++ > configure.ac | 25 ++++++++++--------------- > src/Makefile.am | 10 +++------- > tests/Makefile.am | 15 ++++----------- > 4 files changed, 72 insertions(+), 33 deletions(-) >=20 > diff --git a/CONTRIBUTING b/CONTRIBUTING > index b1f58df..7e3e856 100644 > --- a/CONTRIBUTING > +++ b/CONTRIBUTING > @@ -96,6 +96,61 @@ So, to be complete the regression checking command to = run against your > patch should be: "make check-self-compare distcheck -j16", if you have > a machine with a 16 threads processors, for instance. >=20 > +Coding language and style > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D > + > +The coding style is self evident when reading the source code. So > +please, stick to and mimic what is already in there for the sake of > +consistency at very least. Just for history, it's derived from the > +style of the C++ standard library from the GNU project. > + > +As of libabigail 2.0, the language we use is C++ 11. The level > +supported is the one supported by the GCC 4.8.x series of compilers. > +This should be old and well tested enough to be supported by your > +current favorite compiler. > + > +Initially, the code base of the project was written in C++03, with the > +TR1 extensions. That heritage is well visible in the code base as it > +is today. > + > +Please do not rush and send gazillions of patches that just re-write > +tons of code into your favorite C++ 11 flavour du jour. We will > +likely reject those patches. We want to keep the history of the code > +base in such a way that tools like "git blame are still > +useful. > + > +So we'll accept patches changing parts of the code base to more recent > +C++ 11 constructs only if you happen to add functionality or fix > +things in that area. If it makes "cultural common" sense to adopt > +those constructs. What I mean by "cultural" is that must make sense > +in relative to the culture of the project. And yes, that is > +subjective. Sorry. > + > +As a generic rule, we tend to favor the lowest possible level of > +abstraction that makes sense without requiring future maintainers of > +the project to have a PhD in design patterns. We are not impressed by > +design patterns. We use them where they make clear sense, but we > +don't idolize them. Put it another way, we will always favor the one > +who *READS* and debug the code over the one who writes it. To put > +things in a hypothetical perspective, we'll rather accept a repetitive > +code that stays simple to read and debug over a highly abstract one > +using meta programming to save a few lines of repetitive code located > +in a very small number of places. > + > +Really, in this project, we care about low level binary analysis > +stuff. Issues in that area can be hard to reproduce and quite > +challenging to debug. So having tons of abstraction layers in the > +code base have proven to be a maintenance burden over the years, from > +our experience in working on similar projects. So please help us > +avoid those mistakes that we make just for the pleasure of writing > +what can look as "pleasant code" at a first naive sight. > + > +That being said, we also love cleanly designed APIs that are fairly > +re-usable and well documented. And we also praise abstraction and > +modularisation that we recognize as being the most basic tools of any > +engineer. So we like to think about ourselves as well rounded people > +who care about maintaining things for a long time to come :-) > + > Launching regression tests in Valgrind > -------------------------------------- >=20 > diff --git a/configure.ac b/configure.ac > index 2f15fd0..37b7882 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -85,12 +85,6 @@ AC_ARG_ENABLE(zip-archive, > =09 ENABLE_ZIP_ARCHIVE=3Dno) >=20 >=20 > -AC_ARG_ENABLE(cxx11, > -=09 AS_HELP_STRING([--enable-cxx11=3Dyes|no], > -=09=09=09 [enable features that use the C++11 compiler]), > -=09 ENABLE_CXX11=3D$enableval, > -=09 ENABLE_CXX11=3Dno) > - > AC_ARG_ENABLE(apidoc, > =09 AS_HELP_STRING([--enable-apidoc=3Dyes|no|auto], > =09=09=09 [enable generation of the apidoc in html]), > @@ -160,6 +154,11 @@ AC_LANG([C++]) > AC_LANG_COMPILER_REQUIRE >=20 > dnl > +dnl We use C++11 > +dnl > +CXX_STANDARD=3Dc++11 > + > +dnl > dnl check if the c++ compiler has support __attribute__((visibility("hidd= en"))) > dnl > AC_MSG_NOTICE([checking for GCC visibility attribute support ...]) > @@ -588,14 +587,6 @@ AM_CONDITIONAL(ENABLE_ZIP_ARCHIVE, test x$ENABLE_ZIP= _ARCHIVE =3D xyes) > DEPS_CPPFLAGS=3D"$XML_CFLAGS $LIBZIP_CFLAGS" > AC_SUBST(DEPS_CPPFLAGS) >=20 > -dnl Handle conditional use of a C++11 compiler > -if test x$ENABLE_CXX11 =3D xyes; then > - CXXFLAGS=3D"$CXXFLAGS -std=3Dc++11" > - AC_DEFINE([WITH_CXX11], 1, [Defined to 1 if a C++11 compiler is used]= ) > -fi > - > -AM_CONDITIONAL(ENABLE_CXX11, test x$ENABLE_CXX11 =3D xyes) > - > dnl Check for the presence of doxygen program >=20 > if test x$ENABLE_APIDOC !=3D xno; then > @@ -673,6 +664,9 @@ if test x$ENABLE_UBSAN =3D xyes; then > CXXFLAGS=3D"$CXXFLAGS -fsanitize=3Dundefined" > fi >=20 > +dnl Set the level of C++ standard we use. > +CXXFLAGS=3D"$CXXFLAGS -std=3D$CXX_STANDARD" > + > dnl Check if several decls and constant are defined in dependant > dnl libraries > HAS_EM_AARCH64=3Dno > @@ -949,11 +943,12 @@ AC_MSG_NOTICE([ > C Compiler : ${CC} > C++ Compiler=09=09 : ${CXX} > GCC visibility attribute supported : ${SUPPORTS_GCC_VISIB= ILITY_ATTRIBUTE} > + CXXFLAGS=09 =09 =09=09=09 : ${CXXFLAGS} > Python=09=09=09=09=09 : ${PYTHON} >=20 > OPTIONAL FEATURES: > Enable zip archives : ${ENABLE_ZIP_ARCHIVE= } > - Use a C++-11 compiler : ${ENABLE_CXX11} > + C++ standard level : ${CXX_STANDARD} > libdw has the dwarf_getalt function : ${FOUND_DWARF_GETALT= _IN_LIBDW} > Enable rpm support in abipkgdiff : ${ENABLE_RPM} > Enable rpm 4.15 support in abipkgdiff tests : ${ENABLE_RPM415} > diff --git a/src/Makefile.am b/src/Makefile.am > index d7990e3..26fc4d1 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -4,13 +4,9 @@ libabigaildir=3D$(libdir) >=20 > AM_CXXFLAGS =3D $(VISIBILITY_FLAGS) >=20 > -if ENABLE_CXX11 > -CXX11_SOURCES =3D abg-viz-common.cc=09=09=09\ > -=09=09abg-viz-dot.cc=09=09=09=09\ > +VIZ_SOURCES =3D abg-viz-common.cc=09\ > +=09=09abg-viz-dot.cc=09\ > =09=09abg-viz-svg.cc > -else > -CXX11_SOURCES =3D > -endif >=20 > libabigail_la_SOURCES =3D=09=09=09\ > abg-internal.h=09=09=09=09\ > @@ -42,7 +38,7 @@ abg-tools-utils.cc=09=09=09\ > abg-elf-helpers.h=09=09=09\ > abg-elf-helpers.cc=09=09=09\ > abg-regex.cc=09=09=09=09\ > -$(CXX11_SOURCES) > +$(VIZ_SOURCES) >=20 > libabigail_la_LIBADD =3D $(DEPS_LIBS) > libabigail_la_LDFLAGS =3D -lpthread -Wl,--as-needed -no-undefined > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 6effbc2..655facb 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -3,19 +3,11 @@ SUBDIRS =3D data >=20 > ZIP_ARCHIVE_TESTS =3D > if ENABLE_ZIP_ARCHIVE > -ZIP_ARCHIVE_TESTS +=3D runtestwritereadarchive > -if ENABLE_CXX11 > -ZIP_ARCHIVE_TESTS +=3D runtestdot > -endif > +ZIP_ARCHIVE_TESTS +=3D runtestwritereadarchive runtestdot > endif >=20 > AM_CXXFLAGS =3D $(VISIBILITY_FLAGS) >=20 > -CXX11_TESTS =3D > -if ENABLE_CXX11 > -CXX11_TESTS +=3D runtestsvg > -endif > - > FEDABIPKGDIFF_TEST =3D > if ENABLE_FEDABIPKGDIFF > if ENABLE_RUNNING_TESTS_WITH_PY3 > @@ -55,9 +47,10 @@ runtestlookupsyms=09=09\ > runtestreadwrite=09=09\ > runtestsymtab=09=09=09\ > runtesttoolsutils=09=09\ > +runtestsvg=09=09=09\ > $(FEDABIPKGDIFF_TEST) =09=09\ > -$(ZIP_ARCHIVE_TESTS)=09=09\ > -$(CXX11_TESTS) > +$(ZIP_ARCHIVE_TESTS) > + >=20 > if ENABLE_RUNNING_TESTS_WITH_PY3 > TESTS +=3D runtestdefaultsupprspy3.sh > --=20 > 1.8.3.1 >=20 >=20 > --=20 > =09=09Dodji >=20