FR: Add explicit includes to JUCE modules

[anthony-nicholls]

I said don’t judge me!

[eyalamir]

Haha. Sorry about that!

Anyway the main point is to remove the ‘global’ module header include from the module .cpp and just let the compiler errors guide you through the process.

This would ensure that you’re building a single straight path from each source file into all the declarations that it needs, and that path would be identical in both the compiler and the IDE.

[anthony-nicholls]

@eyalamir

Does that mean you’re adding #pragma once to .cpp files? Don’t do that!

Yep yep I’m quite aware but bare in mind there isn’t actually any difference between a file ending with .cpp or .h, I’ve done this purely for testing purposes.

What it does do is ensure that each single cpp file can be parsed by following the includes alone. Everything should be resolvable by looking at the single file. Keep in mind the headers should already work so in theory this should solve the underlying issue described. I’m not claiming it’s a good solution but it should demonstrate if that is the underlying issue.

The best technique to make this work is stop building the module source file and enable building for all the other cpp files. That’s made more complicated by the fact that it isn’t always clear what is/isn’t a source file due to the structure of many of the internal sources, which is why I recently separated a lot of juce_gui_basics into a detail namespace with lots of headers because separating everything else was a nightmare without it and source files depend on other source files which need to be split into header and source, but then again some of that can be complicated because of it’s dependencies and so on. On top of that sometimes adding the header isn’t the right thing to do because it will subtly change the include order in the main module, instead you really need a forward declaration, which based on what we’re saying wouldn’t be enough for an IDE if it needs to parse everything?

To ensure we keep on top of this and manage it we would really need to add to our CI something that compiles all our module source files individually - which is going to cost us in time.

Ultimately if you approach this too naively there is a risk that you can end up with something that works well enough most of the time, but depending on how you navigate to the file (and the IDE you’re using) the view of the file can subtly change which in turn can lead to more complicated bug reports. In addition more users will rely on including the headers individually which is yet more support overhead.

All of which to be clear we’re saying is worth it so intellisense is happy in VSCode and VS when browsing private JUCE implementations. As far as I can tell CLions issue seems more likely to do with the size of the modules than anything else, and Xcode is just a lost cause it often fails to even find an include that’s right next to the file in ANY project.

Ultimately we want to approach this from the perspective of finding the best solution to a problem, and to do so we need the problem spelled out in concrete terms. If I’ve understood correctly I would say that mot important problem highlighted here is that browsing private source code in JUCE triggers erroneous intellisense warnings in VSCode and VS. This conversation is dragging out because we’re down in the weeds arguing about the “solution”, but what really matters is the problem (or problems).

[anthony-nicholls]

I also didn’t add the extra complexity of ObjectiveC++ being littered around in cpp files, I suspect some IDEs might not like this either if they are trying to parse the file in isolation.

[eyalamir]

@anthony-nicholls

To ensure we keep on top of this and manage it we would really need to add to our CI something that compiles all our module source files individually - which is going to cost us in time.

Please don’t do this. Let me explain:

This would make this work take 10x of what it’s supposed to do.
There’s really no need to have the CI build the code in a way that you’re not deploying to.

It would lead to enormous complexity both in code (two build systems needs to be supported) and tooling/CI since you have to test two ways of deploying the code.

That will just add a big can worms absolutely not needed just to add a few #includes to the JUCE codebase.

Adding includes is trivial. Remove the global include, fix the compile errors with regular includes, done. Start with a simple module so you don’t get overwhelmed by the two beasts (juce_core and juce_gui basics) and learn the super simple principles along the way.

@anthony-nicholls

Ultimately if you approach this too naively there is a risk that you can end up with something that works well enough most of the time, but depending on how you navigate to the file (and the IDE you’re using) the view of the file can subtly change which in turn can lead to more complicated bug reports.

Since I’m currently maintaining about 50 JUCE-style modules, I don’t agree.

Those kind of bugs (red squiggles) are super super rare when a.cpp blindly includes a.h and whatever else it privately needs.

Not saying they don’t happen, but when they do it’s an obvious fix, since instead of full on squiggles, the IDE will tell you something like “Can’t find juce::String” and you can add the include to juce_String.h.

Currently the IDE can’t find anything, so obviously the error messages look like mayhem, but that’s not the case in properly included code, and most IDE errors you’ll encounter will be super clear with good error messages.

[eyalamir]

@anthony-nicholls

Ultimately we want to approach this from the perspective of finding the best solution to a problem, and to do so we need the problem spelled out in concrete terms. If I’ve understood correctly I would say that mot important problem highlighted here is that browsing private source code in JUCE triggers erroneous intellisense warnings in VSCode and VS.

Those errors happen in CLion too just in a different way (no syntax highlighting). Have you seen my screenshot above?

In other JetBrains IDEs like AppCode that are a little bit less ‘smart’ than CLion you actually see some red squiggles in random places in user code too whenever JUCE types are used.

But the real error is the IDE not able to build a correct model of the code. In CLion many functions like “extract method” will fail in JUCE modules without the includes.

But yes - the problem is the red squiggles.

We really don’t have to go into the complexity of each IDE. The problem is that the JUCE code is not correctly structured according to well-accepted C++ practices, which leads to a lot of bugs browsing the code in pretty much all IDEs.

Have you looked at my solution that I’ve added above (the “With Includes” version)?

[Nitsuj70]

I’ll just add that whilst Clion does better than VSCode, at least on my machine (Mac i9 32gb) Clion constantly spends an excessive amount of time trying to build indexes. It’s doing it practically all the time with high CPU and menory consumption, fan whirring etc.

[kunitoki]

I also do this for my modules (some are on the several hundred files between header and cpp) and have nearly no issues with highlighting, navigation or red squiggles, the unity build is just an optimization (don’t care if it’s slightly slower) and the situation is anyway better than how juce modules behave.

Cmake recently released modules support out of experimental… When import juce_core; ?

[daniel]

What I am wondering: will there be a master include for the client code then? Or do you intend to include it like this?

#include <juce_gui_basics/widgets/juce_ListBox.h>

That would be a step backwards IMHO

Just for clarification, my understanding is that only internal sources include directly

[anthony-nicholls]

With the proposal put forward here there would still be a master include but doing what you’ve described would now “work” (which means more users will become dependent on it). I would personally prefer to keep all the work on the the internal cpp files so we can continue not to support that use case, IMO that’s a different feature request.

[daniel]

Yes, that is my preference too.
And if a header inside a module moves, the module internally can update anything, but the client code doesn’t need to know about it.

@anthony-nicholls

but doing what you’ve described would now “work” (which means more users will become dependent on it)

I think it would be dangerous, because they might miss the config file with the preprocessor config settings.

[anthony-nicholls]

@daniel

I think it would be dangerous, because they might miss the config file with the preprocessor config settings.

I wonder if that’s actually an argument for adding the includes as described here, as at least it’s less error prone / dangerous for end-users. But then we already have the issue that sometimes users include these files separately.

I’ll keep thinking about it, for now I think we should concentrate on breaking more of the modules up before we actually do anything (if we do anything).

[eyalamir]

Yes, client code should absolutely not change. In fact that is a requirement IMO that all current demos and unit tests compile as is.

The public header should still exist and include the same things, just that maybe you’ll see less headers there because each header includes its dependencies directly.

The only thing that should change is how the internal include structure of JUCE works.

[anthony-nicholls]

But the headers are part of the public API, adding includes DOES change that in at least two ways.

1. Users could (and will) depend on including the individual header files separately. Due to Hyrum’s Law this will effectively be part of the contract. We already see users do this (hell I’ve done it to work around things in the past!) it will just become more common place. This could be a nightmare for us to maintain as it now means we can’t nearly as easily reshuffle things internally when we want to! Splitting a module would almost definitely be a breaking change.

2. The order of includes may change, looking at the master header will no longer be a good source to understand the order in which things are included. This is subtle but more of a pain for us to manage.

I would prefer to keep the changes to the private implementation files, which is actually where the “problem” is.

[daniel]

Now one could argue if it is a good idea when including a header has side effects towards other includes other than adding types.

I think it is obvious that only the config include should set any defines.

[eyalamir]

@anthony-nicholls

Users could (and will) depend on including the individual header files separately. Due to Hyrum’s Law this will effectively be part of the contract. We already see users do this (hell I’ve done it to work around things in the past!) it will just become more common place.

Again they could (and will) do this now. Only thing this will change is that use case will now work instead of be broken. )

@anthony-nicholls

Splitting a module would almost definitely be a breaking change.

It shouldn’t be for this. If you need to split modules that’s awesome and I have another FR for that. It’s not related to this issue.

An added includes to juce_gui_extra commit should be a non-breaking singular change.

Getting a “major JUCE module refactor” involved as well would make this a massive change that would affect users. Please separate a simple thing like adding #includes from reorganizing the entire codebase.

@anthony-nicholls

1. The order of includes may change, looking at the master header will no longer be a good source to understand the order in which things are included. This is subtle but more of a pain for us to manage.

That’s a bug in the current JUCE layout. I don’t think that’s a ‘feature’, and it’s considered bad practice to do so (see all my links above).

@anthony-nicholls

I would prefer to keep the changes to the private implementation files, which is actually where the “problem” is.

Sure, if you really want to you can keep juce_gui_basics.h as is and just move all the common preprocessor stuff to a common config.h.

However, I think that’s bad practice to do so. A ‘good’ practice would mean that juce_Component.h will include juce_gui_basics.h, and there’s no need to have some global header to create that connection yet again once in the public header once it’s created in the real header.

@anthony-nicholls this change is really so simple… any chance you could just review a PR I’ll send (to one of the simple modules first) to see how dead simple and non-breaking this is?

I feel like in the time we’ve been discussing this we could change the entire JUCE codebase already. We can also change all the simple modules and then make an informed decision on what to do with the 2-3 complicated modules.

[eyalamir]

I really want to be super clear on the change I’m suggesting.

Current situation:

//juce_module.h
#pragma once

#include <juce_other_module.h>
#include "juce_SomeClass.h"

//juce_module.cpp
#include "juce_module.h"
#include "juce_SomeClass.cpp"

Change:

//juce_SomeClass.h
#pragma once

#include <juce_other_module.h>

//juce_SomeClass.cpp
#include "juce_SomeClass.h"

//juce_module.h
#pragma once

#include "juce_SomeClass.h"

//juce_module.cpp
#include "juce_SomeClass.cpp"

This is all I’m suggesting.

User code should not change. All tests/demos should compile. This should be completely separate from other refactoring you may want to do in the codebase otherwise.

Commits can (and should) be as small as possible and only affect one module each time so they can be run in CI in isolation, etc.

[anthony-nicholls]

@eyalamir

Again they could (and will) do this now. Only thing this will change is that use case will now work instead of be broken. )

Which will make more people rely on it meaning internal changes are more likely to break end user code! This isn’t to say it’s wrong but lets at least acknowledge that this is a risk that could come back to bite us later.

@eyalamir

It shouldn’t be for this. If you need to split modules that’s awesome and I have another FR for that. It’s not related to this issue.

It’s not for this but it does impact it, particularly if users rely on it as mentioned above.

Hyrum’s Law is a phenomenon in software engineering whereby developers come to depend on all observable traits and behaviours of an interface, even if they are not defined in the contract

@eyalamir

Getting a “major JUCE module refactor” involved as well would make this a massive change that would affect users. Please separate a simple thing like adding #includes from reorganizing the entire codebase.

I don’t think it needs a major refactor, but some refactoring will be essential to split files into header and source that aren’t already. Unless you want to include .cpp files from other .cpp files because they have interdependencies? Admittedly a lot of this was already done for juce_gui_basics relatively recently so despite being large and relatively complicated I still think this is probably the best module to test against. Most notably because CLion keeps getting used as an argument for this work and I want to actually see it make a difference in that IDE, I think the size of the module still seems like it could be an issue for CLion. My basis for this is that

1. I can already browse small modules such as juce_osc just fine in CLion out of the box without any changes
2. Adding some includes into a file didn’t make it any quicker to navigate to symbols that were defined in the headers included
3. Other developers have reported that they need to increase the RAM usage of CLion
4. It’s the biggest JUCE module and is normally the bounding factor for the speed of building any JUCE project

Anyway the point that I was trying to make is these changes could impact our ability to refactor later. Which we know we need to do anyway, so I would prefer that at least this comes first, particularly because the end user benefits are IMO far greater! I will likely have something up for review today. And I would like to see if we notice any improvement in CLion from doing so.

To be clear I understand you’re suggesting implementing your proposed solution, and that it would be easy to do right now, However,

• I agree it’s relatively easy to implement
• I agree that the commit itself won’t break user code or unit tests
• I agree our modules are too big and so very sluggish to parse
• I agree there is more we could do to make browsing private implementations better
• I agree the proposed solution is a way to improve that browsing behaviour in at least some cases (if not all)
• I don’t agree it’s only a few hours of work to apply to the whole of JUCE
• I don’t agree that no refactoring is required, some internal files will need to be split into header and source files and these will be best placed in a detail namespace (as I recently done for juce_gui_basics) - some cpp files unfortunately currently rely on declarations in other cpp files
• I don’t agree the proposed method of commenting out headers is thorough enough to achieve the end result reliably
• I don’t agree that we shouldn’t add tests for this if we’re accepting it is a “feature” of JUCE.
• I don’t agree that intellisense isn’t presenting buggy behaviour
• I don’t agree that the proposed solution has no downsides whatsoever (ALL solutions have compromises and it’s important we understand and acknowledge them)
• I’m undecided on just how important I think this is right now
• I’m undecided on if I agree the total engineering effort (now and going forward maintaining this) is worth it
• I’m undecided on if I agree if this is the best solution to solve the problem

I think one thing to highlight here is that I think we’re probably just looking at this through different lenses. I’m not just considering what this will look like today, I’m trying to consider how these changes impact us and end users going forward. i.e. how does it impact our ability to add new source or refactor existing source, how do we prevent increasing the number (or complexity) of bug reports, how does it change how users use and rely on JUCE, etc. All of these things are important considerations when you have a large customer base with varying levels of abilities and understanding.

@eyalamir

@anthony-nicholls this change is really so simple… any chance you could just review a PR I’ll send (to one of the simple modules first) to see how dead simple and non-breaking this is?

You’ve already done that, I appreciate it, and I completely understand the suggestion.

@eyalamir

However, I think that’s bad practice to do so. A ‘good’ practice would mean that juce_Component.h will include juce_gui_basics.h, and there’s no need to have some global header to create that connection yet again once in the public header once it’s created in the real header.

IME good or best practice would normally dictate that we use forward declarations wherever possible, and include only those headers that juce_Component.h absolutely requires, it does not require juce_gui_basics.h or everything in it, and it seems to me doing so will likely still be slow in CLion?Interestingly I wonder if forward declarations might also be bad in our case because the IDEs might be slower to resolve the actual symbol?

To be clear when I was talking about a common header I was referring to a header for all the private source files not the public headers, so something akin to juce_gui_basics.h bur for our private source. Again I think I would probably prefer leave the public headers alone if we can, my understanding is that they don’t actually have any issues with intellisense?

@eyalamir

I feel like in the time we’ve been discussing this we could change the entire JUCE codebase already. We can also change all the simple modules and then make an informed decision on what to do with the 2-3 complicated modules.

The reason this conversation is dragging out is because we keep talking about how to implement one specific solution without going back to the actual problem and acknowledging the draw backs to any solution.

I think if we could concentrate on solving the problem we’ll likely come to an agreement a lot faster!!

Let me get this split of gui basics up, let’s evaluate if that has any meaningful impact on CLion. Then lets take a look at applying a solution to the intellisense (and possibly CLion) issue on one of the gui basics modules, and go from there.

[eyalamir]

@anthony-nicholls thank you so much for your thoughtful comments, And I’m happy to see you’re working on this and that we’re generally in agreement!

I just wanted to comment on a couple of things:

@anthony-nicholls

Admittedly a lot of this was already done for juce_gui_basics relatively recently so despite being large and relatively complicated I still think this is probably the best module to test against. Most notably because CLion keeps getting used as an argument for this work and I want to actually see it make a difference in that IDE, I think the size of the module still seems like it could be an issue for CLion.

I totally agree that splitting juce_gui_basics is a good idea for a variety of reasons, and I specifically have an FR for that:

FR: more compact JUCE module headers

We have quite a lot (30+) custom JUCE modules used in multiple products, unit tests, etc, and I noticed that adding any JUCE module include to a module can really inflate compile times, compared to the same module without any JUCE includes. It would be great if some of these high level modules could be split into more compact modules. For example: I’m fine with juce_gui_basics bringing in a bunch of widgets, and no backwards compatibility should be broken for that module. But - in modules wit…

I will say that I’d make this a separate task. First split the module, then do the include stuff. I think even though the two have some correlation they’re not really related.

@anthony-nicholls

I can already browse small modules such as juce_osc just fine in CLion out of the box without any changes

CLion is not a good candidate for checking this, as even though it’s my personal favorite IDE, it’s also a very smart IDE that knows how to parse the build system until it completes the code model.

The problem scope would be much better tested/defined in VSCode and Visual Studio.

@anthony-nicholls

Anyway the point that I was trying to make is these changes could impact our ability to refactor later.

I really don’t see how that’s different from the situation today. You need to make juce_gui_basics.cpp compile and includes for juce_gui_basics.h work.

People including internal headers can happen today too. I’m not sure why adding includes would change that, it doesn’t present any API change.

I also want to stress out that having headers or cpp files including other headers is how every codebase in the world works. I don’t see why JUCE has more refactoring burden because they add includes to files.

I kinda feel like this an argument just for argument’s sake and not a real issue that ever presented itself. In real life, if following a refactor juce_Component.h now needs to include juce_String.h in order to be correct this is a part of the refactoring task anyway, doesn’t matter which file you had to add the include to.

Removing a file/function from a public header or just changing the file structure of public API files would indeed lead to breakage but that again has nothing to do with adding includes…

I’m having a real hard time imagining a scenario where #include your code dependencies has ever caused problem in refactoring.

@anthony-nicholls

I don’t agree that no refactoring is required, some internal files will need to be split into header and source files and these will be best placed in a detail namespace (as I recently done for juce_gui_basics) - some cpp files unfortunately currently rely on declarations in other cpp files

I agree, that one is indeed the ‘annoying’ part of the work that’s required. I just meant that high level refactoring like splitting juce_gui_basics into smaller modules or creating a new JUCE build system is not required.

@anthony-nicholls

I’m undecided on just how important I think this is right now

By no means, I’m not saying it’s something urgent that you have to stop what you’re doing for. But I’m also using CLion where things generally works. If you’ll talk to people using VS Code you’ll get different answers on this topic.

@anthony-nicholls

I don’t agree that we shouldn’t add tests for this if we’re accepting it is a “feature” of JUCE.

Have you ever found a popular codebase that tests for this? My guess is not.
Most codebases I’ve seen just add includes, test that the code compiles and works using the presented build system, and run unit tests on all the different supported APIs.

Once you will remove the global module include, your ‘unit test’ for this would be that the code will not compile if you have a a severe include problem. A minor include problem may still occur, but it would be rare.

I also think that the challenge of unit testing this, as well as maintaining the unit test with JUCE changes is EXTREMELY complicated and non-trivial.

This is something you’re suggesting that will make it very hard for you to refactor, since every change will need to be tested in two different build systems, and not just one, and for very little gain since the compiler will catch all the big problems anyway.

[reuk]

@eyalamir

Once you will remove the global module include, your ‘unit test’ for this would be that the code will not compile if you have a a severe include problem. A minor include problem may still occur, but it would be rare.

Consider updating juce_gui_basics.

• We remove #include "juce_gui_basics.h" from juce_gui_basics.cpp.
• We start adding headers to the inner .cpps.
• We reach juce_Component.cpp and add #include "juce_Component.h" at the top of that file.
• The very next file in the module is juce_ComponentListener.cpp, which uses Component, but that’s already included transitively through juce_Component.cpp, so the compiler does not tell us to add a juce_Component.h include to juce_ComponentListener.cpp. The same goes for all of the following types that use Component in the module.