Closed #2299.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2299#event-2650132716
> if you are having an issue with compilation speed, raise an issue on
> compilation speed so it can be investigated
See #2305
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2299#issuec
@elextr libtool on Windows is the problem with the autotools build, I have no
expectation that it can be made faster. My laptop is very old, raising an issue
will not help that ;-) This pull is just doing what we always did when Enrico
and I maintained the project, not over-including headers unn
@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.
--
You are re
> I don't understand this at all. Prototypes are irrelevant, I don't understand
> what you mean.
It's another kind of forward declarations, and if we start with structs "just
because", where do we stop?
> Please explain what you mean about the struct name changing being bad enough.
> You would
> 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-ma
> I don't know how it is on your machine, but here with a not-to-recent
> laptop, compilation time is not so bad
This pull saves me several seconds build time, which is useful. (That's with
using a resurrected
[makefile.win32](https://github.com/ntrel/geany/commits/makewin32), autotools
on Wi
> 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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on Git
> the intent is to hide the definition of GeanyBuildCommand, then the forward
> declaration should be in build.h
That wasn't the intent because I thought the struct fields were needed by the
API, but I see now they are not needed. The definition could be moved to
build.c. So we would have a for
@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.
> as it trades a tiny bit of compilation time for maintenance and safety costs
How so? There is no safety loss.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2299#issuecomment-531469264
> 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 d
@b4n
> @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.
Maybe I was not precise enough. The ```GeanyBuildCommand``
@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
_Geany
The pointers are only in filetypesprivate.h so they are allocated with each
filetype. They would otherwise be private to build.c.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2299#issue
@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;
> the opposite since it forces you to do forward declaration
How is that a negative vs including an entire header that is not needed?
> 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
> dep
> 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
fo
> As long as the items of struct GeanyBuildCommand shall only be used in
> build.c
Yes, but then they shouldn't be in `filetypesprivate.h` at all. Without
checking I presume something uses them via these pointers, or they should be
removed totally. And if the pointers are being used then the
> 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 insi
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.
@ntrel if you find there is a problem with compile speed raise an issue so it
can be assessed.
--
You
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,
I think it's fine to keep it locally while you work intensively on the build
stuff but IMO it's not a general improvement (actually the opposite since it
forces you to do forward declaration). Maybe others think different though.
--
You are receiving this because you are subscribed to this thre
@kugel- There are about 3 files that include filetypesprivate.h but not
build.h. This speeds up rebuilding after changes to build.h.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2299#is
I'm not seeing how this is an improvement, to be honest.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2299#issuecomment-531177608
@ntrel pushed 1 commit.
f5c09882b170fe14edeebacf7b3047383a4e31c6 Fix missing build.h include
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/geany/geany/pull/2299/files/df0073a3a646ed669e96ab1ae18ae16fa5f35f57..f5c09882b170fe14edeebacf
You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2299
-- Commit Summary --
* filetypesprivate.h: Don't include build.h
-- File Changes --
M src/filetypesprivate.h (20)
-- Patch Links --
https://github.com/geany/geany/pull/2299.patch
27 matches
Mail list logo
