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

Mass-rename all relevant sources to a C++ extension #3316

Open
pmatilai opened this issue Sep 19, 2024 · 25 comments · May be fixed by #3356
Open

Mass-rename all relevant sources to a C++ extension #3316

pmatilai opened this issue Sep 19, 2024 · 25 comments · May be fixed by #3356
Labels
build Build-system related cleanup

Comments

@pmatilai
Copy link
Member

pmatilai commented Sep 19, 2024

4.20 release is nigh (#3271) so as outlined in #3103, the time to do a mass-rename is closing in, this is simply a ticket to allow tracking + planning it. And maybe discuss a bit too.

The main "issue" here is deciding which suffix to use. From what I've seen, .cpp looks like the most common one for modern c++ code and would be in line with dnf5 which doesn't hurt either.

The other question is headers: we'll be dealing with plain C headers from here to eternity, so it seems like it'd be helpful to differentiate what is meant for C vs C++ consumption. .hpp seems like a consisntent and logical suffix there, and also what dnf5 uses. Not that it dictates in any way, but doesn't hurt either because it's a neighbor team really.

AC:

  • C++ sources are renamed to .cc and C++ -only (ie those without __cplusplus guards) to .hh. Exempt from this rename are
    • python/
    • plugins/
    • lib/backend/ndb/rpm*.*
    • misc/
  • hacks to force cmake to use C++ compiler on .c sources are removed
  • a freshly configured tree builds, tarball creation works and the test-suite passes

Edit: the initial plan was to use cxx/hxx to match the common variable naming in build systems but the result is not something we can stand looking day after day. Seriously.

@pmatilai pmatilai added cleanup build Build-system related labels Sep 19, 2024
@Conan-Kudo
Copy link
Member

I'm fine with .cxx/.hxx or .cpp/.hpp. I have a slight preference for .cxx/.hxx simply because it disambiguates with the C preprocessor and it also lines up with the usage of CXXFLAGS as the environment variable for C++ compiler flags.

@pmatilai
Copy link
Member Author

I don't think I've ever come across a .cxx/.hxx extension in any project that I've looked at, but that is a totally fair point.

@Conan-Kudo
Copy link
Member

CMake itself uses it, and when I first learned C++, I used it for my stuff too. 😅

@Conan-Kudo
Copy link
Member

You can also use .c++/.h++ if you're feeling bold... 😛

All the valid extension types are listed on the Wikipedia page for the language.

@dmnks
Copy link
Contributor

dmnks commented Sep 25, 2024

I'm also more accustomed to seeing .cpp/.hpp but Neal's point about the CXXFLAGS is valid. Also, it seems like .cxx/.hxx is ever so slightly more common (historically) on UNIX whereas .cpp/.hpp on Windows (based on a quick search on the interwebz). I also kinda like the more minimalistic .cc/.hh or even .C/.H 😄

That said, .c++/.h++ rubs me the wrong way for some reason 😅

@Conan-Kudo
Copy link
Member

Conan-Kudo commented Sep 25, 2024

I'm not a fan of .cc/.hh simply because it looks like it's associated with the traditional UNIX C compiler binary cc.

And case sensitive extensions are evil. No. 😭

@dmnks
Copy link
Contributor

dmnks commented Sep 25, 2024

Oh, I've just realized that the xx is actually the slanted ++ 😄 I see what they did there.

@dmnks
Copy link
Contributor

dmnks commented Sep 25, 2024

I'm not a fan of .cc/.hh simply because it looks like it's associated with the traditional UNIX C compiler binary cc.

Oh, yup. It's also just two characters and somehow that feels wrong. Like, how many file extensions are there with only two characters...

And case sensitive extensions are evil. No. 😭

Yeah, that was mostly a joke, please don't choose those 😄

@Conan-Kudo
Copy link
Member

Also note that C++ modules will use .ixx, and if we ever get around to using them for an RPM C++ API interface (in some indeterminate time in the future), it would be nice if everything lined up for it.

@pmatilai
Copy link
Member Author

Also note that C++ modules will use .ixx

Where is that stated? Based on quick googling around, .ixx seems to be a Visual Studio specific extension. Both gcc and clang have/accept their own litanies (.cppm, .ccm, .cxxm, .c++m and whatnot) 🤪

@Conan-Kudo
Copy link
Member

Oh geez, okay whatever, strike that as a reason. 😵

Still, I stand by my original rationale for cxx/hxx.

@pmatilai
Copy link
Member Author

Yup, cxx/hxx has the significant benefit that it's the only version with some actual, concrete rationale to back it 😆

I was a little surprised to see the core guidelines recommending .cpp and .h. .cpp is common enough not to raise eyebrows, but .h? Why on earth would one NOT want to differentiate from plain C headers?

@Conan-Kudo
Copy link
Member

It's somewhat rare these days to have mixed C/C++ headers in a single project, so I can see why they say that. That doesn't mean I agree with it. 😃

@dmnks
Copy link
Contributor

dmnks commented Sep 25, 2024

Why on earth would one NOT want to differentiate from plain C headers?

Isn't that to have a single header file for both C and C++ usage, assuming that you'd wrap the C++ only bits with __cplusplus?

Edit: Basically what we do right now in our header files 😄

@pmatilai
Copy link
Member Author

As for sharing the same header - yes you can certainly do that. AFAIK neither C or C++ compilers are actually in the slightest interested about the extension, that's all just convention. I would instinctively pick .h for plain C.

We have a specific reason to not mix the headers: we don't want to make our C++ API public anytime soon. Having C++ in separate headers also helps editors do syntax highlighting and the like.

@pmatilai
Copy link
Member Author

pmatilai commented Sep 25, 2024

cxx/hxx has the significant benefit that it's the only version with some actual, concrete rationale to back it

Once you put it down this way... it'd seem foolish to do anything else really. So .cxx and .hxx it is. Thanks for the input!
Note that .hxx for C++ only headers - so at this point nothing in include/rpm will change.

@dmnks
Copy link
Contributor

dmnks commented Sep 25, 2024

Ack, it seems like sticking to .h for pure C headers and .hxx for C++ (later) makes the most sense, and is most consistent.

@dmnks
Copy link
Contributor

dmnks commented Sep 25, 2024

We have a specific reason to not mix the headers: we don't want to make our C++ API public anytime soon. Having C++ in separate headers also helps editors do syntax highlighting and the like.

Indeed, if it quacks, it's a duck. No point in pretending otherwise 😄

@pmatilai
Copy link
Member Author

Added an AC to the description - lets try to get that habbit rolling too 😄

@Conan-Kudo
Copy link
Member

cxx/hxx has the significant benefit that it's the only version with some actual, concrete rationale to back it

Once you put it down this way... it'd seem foolish to do anything else really. So .cxx and .hxx it is. Thanks for the input! Note that .hxx for C++ only headers - so at this point nothing in include/rpm will change.

Makes sense to me! Let's do it!

@pmatilai
Copy link
Member Author

pmatilai commented Oct 3, 2024

I'm in the middle of doing this, and seeing all these exes in practise... its just SO EYE-BLEEDING UGLY that I cannot imagine living with this from here on:

        manifest.cxx manifest.hxx package.cxx
        poptALL.cxx poptI.cxx poptQV.cxx psm.cxx query.cxx
        rpmal.cxx rpmal.hxx rpmchecksig.cxx rpmds.cxx rpmds_internal.hxx
        rpmfi.cxx rpmfi_internal.hxx
        rpmgi.hxx rpmgi.cxx rpminstall.cxx rpmts_internal.hxx
        rpmlead.cxx rpmlead.hxx rpmps.cxx rpmprob.cxx rpmrc.cxx
        rpmte.cxx rpmte_internal.hxx rpmts.cxx rpmfs.hxx rpmfs.cxx

@pmatilai
Copy link
Member Author

pmatilai commented Oct 3, 2024

Making you all cross-eyed is one thing, but those exes actually obscure relevant information: whether a file is a header or a source gets all but lost in there.

@pmatilai
Copy link
Member Author

pmatilai commented Oct 3, 2024

For a side-by-side comparison of the same thing:

fprint.cxx fprint.hxx tagname.cxx rpmtd.cxx tagtbl.inc
cpio.cxx cpio.hxx depends.cxx order.cxx formats.cxx tagexts.cxx fsm.cxx fsm.hxx
fprint.cpp fprint.hpp tagname.cpp rpmtd.cpp tagtbl.inc
cpio.cpp cpio.hpp depends.cpp order.cpp formats.cpp tagexts.cpp fsm.cpp fsm.hpp
fprint.cc fprint.hh tagname.cc rpmtd.cc tagtbl.inc
cpio.cc cpio.hh depends.cc order.cc formats.cc tagexts.cc fsm.cc fsm.hh

Those of us who work on this on daily basis need to think of their health, both mental and physical. There's just no comparison between those three, we'll take the last one and run with it. It's actually a pretty common choice - for example gcc uses .cc/.hh.

@dmnks
Copy link
Contributor

dmnks commented Oct 3, 2024

Truly, we've needed to see this from another perspective to notice. Nothing wrong with adapting plans accordingly.

@pmatilai
Copy link
Member Author

pmatilai commented Oct 3, 2024

And nothing in this world prepared me for the onslaught of those exes in the masses 🙈
It's just a wall of blur in practise 😵‍💫

pmatilai added a commit to pmatilai/rpm that referenced this issue Oct 3, 2024
As planned from the start of the C++ journey, rename all C++ files to
a C++ extension to remove the confusion for both humans and tooling -
so we now get to remove the associated cmake hacks to force it to
use a C++ compiler.

We initially decided on going for .cxx/.hxx extension on the grounds
that it's the technically most right thing to do as it matches with
the build toolchain CXX/CXXFLAGS and all that. But at around 1/4 way
through the rename I concluded that this is not only eye-bleeding ugly,
it also totally obfuscates the difference between headers and sources
in the visual noise it creates. Last but not least, it makes you think
of All My Ex's Live in Texas, and that's about the last thing in I
want to be thinking off. Both the song, and the exes.

Those of us working on rpm for a living need to think of their
well-being, both mental and physical. The choice of .cc/.hh extension
is primarily based on that: the .cxx/.hxx extension proved to be almost
revolting in practise. To my total surprise really, I never expected
this to matter at all. Maybe others have had similar feelings though,
.cc/.hh is one of the more common choices, for example gcc and binutils
use that.

Fixes: rpm-software-management#3316
@pmatilai pmatilai linked a pull request Oct 3, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build-system related cleanup
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

3 participants