PortMidi / portmidi

portmidi is a cross-platform MIDI input/output library
Other
118 stars 33 forks source link

Add include guards to all headers #64

Closed fwcd closed 7 months ago

fwcd commented 8 months ago

To avoid including headers multiple times, this PR adds include guards to all headers (a standard practice in C/C++).

[!NOTE] The alternative implementation would be to use #pragma once, a non-standard, but more concise directive. Since portmidi.h already contained an include guard, I went with this option. Let me know which you prefer.

rbdannenberg commented 8 months ago

I agree this is a standard practice in C/C++, but I don't think it's a good one, so I avoid it. The problem is that it leads to including things where includes are not necessary, easy on code writers, but giving code readers very little help in discovering dependencies among modules. Code reading support is much more important than code writing support. However, I'm not going to convince people they're wrong, so I should probably just include the guards. This is a really busy time now, so it might have to wait a month or so before I can really look at it. Thanks for the contribution.

fwcd commented 8 months ago

Thanks for the note! No worries about being busy.

I am not quite sure I follow the reasoning though. Yes, people often include more stuff than necessary, but even a project where every header includes exactly its dependencies can exhibit the problem of duplicate includes. To illustrate:

if my project has a.h and b.h, which in isolation both depend on some non-include-guarded library.h, and c.h happens to depend on both a.h and b.h, then each header is declaring exactly its dependencies, yet library.h gets included twice. Not including library.h in a.h and b.h would effectively force every consumer c.h to diligently remember to include library.h first, which IMO is more brittle (by imposing an implicit contract) than just letting each header include exactly what it depends on and using guards/pragma once to solve the double include problem.

I agree that it can look a bit noisy though and, unless some very esoteric compilers are to be supported, #pragma once would be a leaner alternative that is supported by all major compilers. Since it's personal preference, I'd be fine either way.

rbdannenberg commented 7 months ago

I agree with your example and analysis: "my" way forces every consuming .c file must diligently include the transitive closure of all dependent header files. It's painful, but later, when you're trying to understand the code, I find it really helpful to see all the dependencies. Another way this helps is this style makes it unlikely that someone that's trying to write portable code can accidentally make calls to functions that PortMidi is trying to hide, e.g. if you might get away with AudioGetHostClockFrequency if porttime.h included CoreAudio/HostTime.h instead of ptmacosx_mach.c (maybe not the best example). PortMidi is pretty stable and I don't think people are pulling out and using subsystems -- there really aren't any -- so let's leave it in the brittle form.

fwcd commented 7 months ago

Another way this helps is this style makes it unlikely that someone that's trying to write portable code can accidentally make calls to functions that PortMidi is trying to hide, e.g. if you might get away with AudioGetHostClockFrequency if porttime.h included CoreAudio/HostTime.h instead of ptmacosx_mach.c (maybe not the best example).

That's a valid point and the usual way to solve that would be either not including the Core Audio header in the .h (unless it actually is part of the public API, otherwise including it in the source file would be sufficient) or adding a private header that is only used internally.

The reason why it currently "works" is that PortMidi headers are fairly minimal and do not really have many dependencies. Unless the API surface grows, that may work out, but it probably isn't a particularly scalable way to structure the code.

rbdannenberg commented 7 months ago

Like I said, maybe not the best example, but things like that do come up. Also, making all the dependencies visible often provides insight: "Why am I including this file? If I need this file, there's something I don't understand...." and I'm not sure silently sucking in a big tree of .h files because lower levels have not done much information hiding is very scalable either. I've used this with 50K-line programs but not 1M-line programs, so I'm not qualified to talk about scaling. I've also used header guards, and we used them in Audacity, so it's mainly just a personal preference, and I'm in the minority.

rbdannenberg commented 6 months ago

I'm integrating various updates and decided to include the include guards for public headers portmidi.h (already there, but I changed names according to your more consistent ones), pmutil.h and porttime.h. The other .h files are internal and should not be included by any code outside of PortMidi.