@ntrel pushed 1 commit.

f5c09882b170fe14edeebacf7b3047383a4e31c6 Fix missing build.h include

@kugel- There are about 3 files that include filetypesprivate.h but not build.h. This speeds up rebuilding after changes to build.h.

b4n requested changes on this pull request.

I have to agree with @kugel- here: if the file uses a symbol defined in a header, it should include it. Manually introducing forward declarations should IMO be left to breaking circular dependencies alone. Also, I don't know how it is on your machine, but here with a not-to-recent laptop, compilation time is not so bad I feel we need to resort to such workarounds.

This doesn't make any sense, the point of including headers is so that definitions are available to compilers for checking and optimisations. By making them opaque pointers that is defeated.

I disagree. If items inside ```struct GeanyBuildCommand``` are private and shall only be used inside ```build.c``` then it is practicable to use a forward declaration and hide the struct items. No code outside ```build.c``` can then use the struct in a wrong way. Of course this requires functions to access/use all aspects of ```struct GeanyBuildCommand```. As the ```struct GeanyFiletypePrivate``` only uses pointers to ```struct GeanyBuildCommand``` the compiler has all information it needs and can still do type checks etc. (it is still a ```struct GeanyBuildCommand *``` not a ```void *```). If something was missing then there would be a compiler error. But I would prefer a forward declaration with typedef.

As long as the items of ```struct GeanyBuildCommand``` shall only be used in ```build.c``` and if it shall stay like that in the future then I actually consider this good style.

Yes, but then they shouldn't be in ```filetypesprivate.h```

Why? Even if the struct items are not accessed then you still need the pointers to the memory for the filetype specific build commands. So I agree we might need to include ```build.h``` for the function declarations but I like the forward declaration to keep the header file as small as possible.

@elextr: I did a code search in Geany of these pointers: ``` /* TODO: move to structure in build.h and only put a pointer here */ GeanyBuildCommand *filecmds; GeanyBuildCommand *ftdefcmds; GeanyBuildCommand *execcmds; GeanyBuildCommand *homefilecmds; GeanyBuildCommand *homeexeccmds; GeanyBuildCommand *projfilecmds; GeanyBuildCommand *projexeccmds; ``` They are all used only with ```g_free```, ```SETPTR``` or direct address comparison. No one of that is passed to any function declared in ```build.h``` outside of ```build.c```.

@LarsGit223 at least `projfilecmds` is being accessed outside `build.c`, in `project.c`, so it's not actually completely private to `build.c`. If it was, build.h should have had the forward declaration, and `build.c` the complete one.

@ntrel `struct _GeanyEntryActionPrivate` and `struct _GeanyMenubuttonActionPrivate` are forward declarations because the definition is totally private, in the corresponding source file (that includes its header). This is perfectly fine IMO because not only it's the only way to make a structure totally opaque, but the forward declaration is also included by the code having the definition, so the compiler can do a check that those are compatible. Some of the ones you mentioned are to break circular dependencies (e.g. search.h's one looks like that), so we don't have much choice. All others that aren't required for breaking circular dependencies should be removed and the proper header included IMO.

The point of including headers explicitly per-file is to reduce dependencies on source files.

It also helps controlling what uses what, and have more generic parts not entangled between them. But that's not really the point IMO, we could have a single header and that'd be mostly fine by me. What matters IMO is that declarations can be checked against their definition, to avoid conflicts and such. If declarations start popping everywhere, it makes it a *lot* harder to change them or check if they make sense. OK a structure can only change name (which is bad enough), but if we'd extend this to e.g. prototypes it's diving into madness if that function signature becomes incorrect. So basically my take is, as much as possible, keep only one declaration (in the .h) to have minimal effort when modifying it, and have that declaration be used together with the definition (.h included in the .c) so the compiler can actually check the declaration is correct for us.

This makes this change not worth it IMO, as it trades a tiny bit of compilation time for maintenance and safety costs. That's not a deal I'm wanting to make.

If declarations start popping everywhere, it makes it a lot harder to change them or check if they make sense. OK a structure can only change name (which is bad enough), but if we'd extend this to e.g. prototypes it's diving into madness if that function signature becomes incorrect.

I don't understand this at all. Prototypes are irrelevant, I don't understand what you mean. Please explain what you mean about the struct name changing being bad enough. You would get a compile error.

@LarsGit223 As @b4n said, if the intent is to hide the definition of GeanyBuildCommand, then the forward declaration should be in build.h (perhaps along with a typedef) to express that intent. With this patch, the intent is rather "just avoid including build.h" instead of making an opaque type.

The definition could be moved to build.c

I probably won't do that, this pull is more important to me. It's possible that struct might be useful to access outside build.c later.

That's with using a resurrected makefile.win32, autotools on Windows is painfully slow

After `configure` - which _is_ super slow on Windows, but rarely needs to be run unless you're hacking the build system - it's just running a Makefile anyway and shouldn't be very different from a hand-made Makefile if it's performing the same build tasks.

@ntrel as I said, if you are having an issue with compilation speed, raise an issue on compilation speed so it can be investigated, nobody else is complaining of compile speed, so maybe its something in your setup.

Don't just raise a random PR with what you think is the solution.

if you are having an issue with compilation speed, raise an issue on compilation speed so it can be investigated

See #2305