Le 22/02/2011 20:04, Thomas Martitz a écrit :

Am 22.02.2011 19:36, schrieb Colomban Wendling:

...

...

So my 1st suggestion is to remove all plugins which do have known issues and don't compile with some -W-flags (needs to be defined) from common build until these are fixed. Also remove plugins which don't bring propper documentation as well are unmaintained for some time.

Well... I'm puzzled. Disabling unperfect plugins is a good idea in theory for the stability of Geany, but... how would be plugins tested then?

However, I agree with making official builds with strong error checking enabled (at least -Werror-implicit-function-declaration comes directly to my mind). This is static checking that will show most obvious problems, and hopefully it'll encourage their maintainer to fix them. However, finding the right flags will not be easy... for example not allowing any implicit cast may have a false-positive effect where unexperimented developers might hide a part of those with a cast where a proper fix would be better.

I agree. While we may know they have known problems, how would we know in the future if they're disabled and receive no testing?

I think all plugin developers should get more easy about others touching there code. I could probably fix a number of problems in other people's plugins, but I'm afraid of the surrounded discussion (post or pre commit).

I don't think that touching other one's code without her agreement is a good idea. There are several reasons for this that comes to mind: * some people will find this offensive (e.g. "you can't even code right and you won't even understand if I tell you"); * some people will find it cool, but won't necessarily try to understand any further (heh, it's already fixed), so would likely reproduce mistakes later; * and unfortunately, some people might change things with not enough care or understanding and break other things -- this is easier than we might think... * and some people would not feel confident of somebody else commit -- and this would probably be my case if it's somebody I hardly know.

And AFAICT, people who would need help are generally likely to accept it when proposed (probably more than if it was imposed).

What I also wonder; is there a way to prevent plugins from crashing geany entirely? I'd rather have a notice "Oops, Plugin Foo crashed".

I doubt. This would need to run plugins in a separate process, and then doing some IPC to communicate. Probably doable (well, AFAIK recent web browsers tend to do so with their plugins -- chromium, FF4) but probably really not easy.

Regards, Colomban

Le 22/02/2011 22:30, Thomas Martitz a écrit :

Am 22.02.2011 21:40, schrieb Colomban Wendling:

...

Le 22/02/2011 20:04, Thomas Martitz a écrit :

...

Am 22.02.2011 19:36, schrieb Colomban Wendling:

...

...

So my 1st suggestion is to remove all plugins which do have known issues and don't compile with some -W-flags (needs to be defined) from common build until these are fixed. Also remove plugins which don't bring propper documentation as well are unmaintained for some time.

Well... I'm puzzled. Disabling unperfect plugins is a good idea in theory for the stability of Geany, but... how would be plugins tested then?

However, I agree with making official builds with strong error checking enabled (at least -Werror-implicit-function-declaration comes directly to my mind). This is static checking that will show most obvious problems, and hopefully it'll encourage their maintainer to fix them. However, finding the right flags will not be easy... for example not allowing any implicit cast may have a false-positive effect where unexperimented developers might hide a part of those with a cast where a proper fix would be better.

I agree. While we may know they have known problems, how would we know in the future if they're disabled and receive no testing?

I think all plugin developers should get more easy about others touching there code. I could probably fix a number of problems in other people's plugins, but I'm afraid of the surrounded discussion (post or pre commit).

I don't think that touching other one's code without her agreement is a good idea. There are several reasons for this that comes to mind:

• some people will find this offensive (e.g. "you can't even code right

and you won't even understand if I tell you");

• some people will find it cool, but won't necessarily try to understand

any further (heh, it's already fixed), so would likely reproduce mistakes later;

• and unfortunately, some people might change things with not enough

care or understanding and break other things -- this is easier than we might think...

• and some people would not feel confident of somebody else commit --

and this would probably be my case if it's somebody I hardly know.

And AFAICT, people who would need help are generally likely to accept it when proposed (probably more than if it was imposed).

It's all open source after all, and part of it is working together with others on the same code. All of your points contradict with that spirit.

I don't think so. In general, open source projects are maintained by a few people that know (and hopefully trust) each other (hey, at least they worked together for a while) and third-party code is contributed by patches the maintainers have a chance to review. So it's not exactly the same picture here I think since we have one repository for several projects.

What I mean is basically: I personally would welcome any patch and be happy to review it, but I would not accept the same code changes to happen without asking me. This is NOT an arrogant position, it's more a matter of responsibility: if somebody is the maintainer of something, she must accept the changes made to that thing, because she'll be responsible of what happens if the change was bad.

This is exactly the same picture that sometime can be seen in OSS projects when proposing a patch that introduces large change: the people might appreciate it, but they gotta maintain it, or know you'll be still around to maintain it over time. So maybe they will refuse it only because of the maintenance overhead. It's maybe sad, but not necessarily unwise.

I didn't mean to say I would like to do work on someone else's plugin (as in new features). Just fix the most immediate problems. If the fix not a few-liner or no brainer I wouldn't bother with it any further anyway (but instead just disable the plugin for the time being).

I understand your purpose, but I doubt that such a policy would please anyone. Yes, I agree that IF the people that commit those fixes you talk about are only the ones that are competent to do so, it'd be acceptable IMHO. But with such policy it's likely that at some point somebody will feel too confident and "fix" somebody else's code wrongly. And there will be the drama, the hard feelings and the collapsing of the whole project -- I know, it's a pessimistic POV :D

Cheers, Colomban

Le 22/02/2011 23:39, Thomas Martitz a écrit :

...

...

I didn't mean to say I would like to do work on someone else's plugin (as in new features). Just fix the most immediate problems. If the fix not a few-liner or no brainer I wouldn't bother with it any further anyway (but instead just disable the plugin for the time being).

I understand your purpose, but I doubt that such a policy would please anyone. Yes, I agree that IF the people that commit those fixes you talk about are only the ones that are competent to do so, it'd be acceptable IMHO. But with such policy it's likely that at some point somebody will feel too confident and "fix" somebody else's code wrongly. And there will be the drama, the hard feelings and the collapsing of the whole project -- I know, it's a pessimistic POV :D

Yes, very pessimistic. I'm a bit more optimistic and have trust that people can at least consider their competence before touching and committing to foreign code.

One might say it's only an optimistic POV ;)

And even then, code changes can always be undone.

That's true. Thanks to VCS :)

Perhaps I'm just biased by how slow things are currently going in the Geany core, where more collaboration (by more people) would help a lot (I even think it reached a dangerous level of standstill).

I can't disagree with this... but if maintainers have less time to work, I can't really blame them. As I said in a previous mail, IMHO the maintainers are quite responsible of how the project works; so I easily understand that they don't want to give commit access to somebody they don't trust. Though, perhaps they might think a little about it and see if there is some people that would be acceptable (I think of Lex in the first place, though I can't tell he's interested). But this is another discussion.

But I always think the more people the merrier.

That's generally true, assuming these people are smart enough to know their own capabilities.

Currently every plugin author does his own thing with their plugins and communicate very little about it.

I'd like it better if each plugin had a main maintainer and everyone else would be a co-maintainer. Now we have one and only one person working on each.

Don't get me wrong: I'm not against it. Why not. Actually, as far as everything goes right, I'm happy with your proposal. But I also fear what can happen if we don't explicitly ask the author.

A idea might simply be to ask every plugin author (and keep a list of their answer not to have to ask again and again) if they are OK with other people maintaining/developing/fixing/whatevering it. Perhaps even stricter, who they allow to work on their plugin. I guess this would be a good compromise, and would avoid hard feelings since its the author that chooses.

Regards, Colomban

Hi all,

For first thing, maybe we could enforce use/passing of those tools mentioned and these before adding to release, examples: http://www.splint.org/ http://valgrind.org/info/tools.html (suppression for GTK - http://people.gnome.org/~johan/gtk.suppression) http://www.gnu.org/software/indent/ (just for making coding styles more consistent between plugins) http://check.sourceforge.net/ or http://cutest.sourceforge.net/ or http://cunit.sourceforge.net/ Perhaps some or all of these could be automated.

Another thing could be to make mandatory that documentation is existent and current, up to some standard. I mean for README, manual, and also doc-comments in code (ex. each function/global must have a comment or something). Some other items where documentation could be enforced; new keybindings, preferences, menu/toolbar items, tabs.

In reference to using additional languages for plugins, I think this should be #1 priority for improving quality. Think about it like this; who writes plugins? Geany users/hackers. What languages do they use/know? Probably not C in a lot of cases (more likely Python, PHP, etc - of course I'm just guessing on this). To make a PHP programmer write a plugin in C is asking for trouble (unless they know C also). If you could use a scripting language like Python, it would basically totally eliminate crashing Geany. I can't think of anything you could do in plain Python that would crash the interpreter all-together (thought it might be possible).

Of course this brings back the problem of the plugin API not being well-suited to use in other languages (no shared library to link to, no way to auto-generate the large bindings required, having to maintain backwards compatibility, etc). I was the one working on GeanyPy to try to add Python bindings but I all but gave up due to the above issues.

I think a big step forward would be to add the Geany Vala bindings into the official code distribution (adding to autotools/waf to put the VAPI into the right system location). Of course the Vala binding needs testing, but I can verify that it's very usable, having written some plugins with it. It won't get much testing unless it's part of Geany and mentioned in the Plugin documentation so people know it exists.

In the sense of VCS, I think using a more "social" site than SourceForge for the plugins could be helpful (I'm thinking GitHub, Gitorious, etc). There could be a geany-plugins repository that is the official one containing releasable code. There could be additional repositories each plugin author maintains against the official repository, where they make all their in-between release changes, and at release time, a pull request for each plugin could need to be sent to the official repository to bring in the releasable version. The benefit of this is also that you could hack on a plugin by forking it and then send a pull request to the person who maintains the plugin's repository for them to review, without messing with their code directly. (More info: http://help.github.com/pull-requests/) I know at least for GitHub, there is provided very pretty graphs and user interface which shows all issues, fork queue, pull requests, and other work being performed on the same code across repositories. This might be personal preference for workflow, because I'm not adept at using Subversion and I don't have access to make changes to the project at SourceForge, but it does seem to be the way the "cool kids are doing it" (ex. https://github.com/mirrors/linux-2.6).

Another slightly off topic thing that could be done is to make the plugins installable from within Geany where a releasable plugin hosted officially could be downloaded/updated by itself (kinda like Eclipse) without requiring all the stress of making a massive multi-plugin release for each Geany release. This would also make less work for whoever has to keep the build-systems up to date with the plugins. If the specific plugin author doesn't ensure their plugin builds properly, then people just won't be able to use that plugin. Also plugins could be fixed without waiting for release time.

Ok, I have typed too long now, so I will stop here :)

Cheers, Matthew Brush (codebrainz)

Am 23.02.2011 04:01, schrieb Lex Trotman:

Of course on the other hand because the maintainers are volunteers with limited time this process slows changes, but IMHO this is necessary. Having more maintainers for a piece of code is of course the solution, but as we are finding, its hard to actually do for a small project. Even for core there was a deafening silence when I recently suggested that more people volunteer to be maintainers.

I don't think that silence meant anything other than the people that can decide haven't read it yet. But yea, I would welcome a discussion about this in the core. There's great potential contributors on this list, but they're all sitting on their patches and nothing moves forward.

Of course the Geany community also has a responsibility to Geany users (not least ourselves :-) to make sure that the whole Geany ecosystem offered as an "official" release meets reliability and usability standards. On this basis, Frank's concern that some plugins don't meet those standards is important, but I don't think that it is reasonable for the community to impose work on maintainers without discussion (thats this thread) and a reasonable period for improving things, otherwise the community is undermining the maintainers responsibility, and I certainly would get upset if I were a maintainer.

You seem to have misunderstood that letting other people help fix bugs is voluntary is has nothing to do with undermining. And I didn't suggest to share maintainership completely, but just accept help to fix the most immediate bugs (such as crashes) in a timely manner. Nobody wants to take plugins away from developers.

Best regards.

If nobody mind, let me state my opinions:

1. Maintaining I believe there has to be only one maintainer who is commiting code. As for me, the amount of code in a ordinary geany plugin is not that huge, one can easily support it. Others who has patches/improvements have to send them to the maintainer and it's he, who decides what to do with them and is somehow responcible for the result. Giving commiting rights to several people can cause mess in code. If the plugin is unmaintained for a long time, lets contact a developer to learn his plans and if he hasn't time for now (or at all) and there is someone else who wants to do his job - let this man to become a new maintainer, I think little developers will reject this suggestion if they really have no time to deal with the code for now. Current active maintainer have to mentioned on the plugins web-site in order to contact him to solve problems / send patches etc

2. Documentation Support Mathew, there has to be common documentation system, maybe we can start from standartization README etc standart files

3. Different repos Here I strongly disagree, I think this will also cause kind of a mess, why not to solve problems in te common repository?

4. Removing unsupported plugins from releases what do you think about the following scheme: divide all pluging into: - "supported" (that are acting really well) - "unsupported" or "bad" (having problems) ? So, every geany-plugins release will contain several packages: "geany-plugins", "geany-plugins-bad or "geany-plugins-unsupported", something like this

5. Other language bindings - don't really think it can increase plugins quality dramatically, there can be problems in any language that you have to solve in order to make your code work correctly.

And one more thing, as a debian user I see that there is still 0.19 plugins version even in unstable, maybe it's a good idea to move current developing version (0.21?) to unstable / testing to make debian users to help us in bug reporting?

Cheers, Alex

On Wed, 23 Feb 2011 12:41:54 -0800 Matthew Brush matthewbrush@gmail.com wrote:

...

4. Removing unsupported plugins from releases

what do you think about the following scheme: divide all pluging into:

• "supported" (that are acting really well)
• "unsupported" or "bad" (having problems) ?

So, every geany-plugins release will contain several packages: "geany-plugins", "geany-plugins-bad or "geany-plugins-unsupported", something like this

This is like GStreamer plugins and I think it's a very good idea. They could be grouped in the configure.ac file to add an option like --enable-bad and --enable--unsupported or something.

How this is going to be maintained? Somebody has to take over responsibility to set the flag.

Cheers, Frank

On Wed, 23 Feb 2011 15:28:05 +0300 Alexander Petukhov Alexander.Petukhov@mail.ru wrote:

And one more thing, as a debian user I see that there is still 0.19 plugins version even in unstable, maybe it's a good idea to move current developing version (0.21?) to unstable / testing to make debian users to help us in bug reporting?

I did see Geany 0.20 has been migrated. I'm sure the plugins will follow soon.

On Wed, 23 Feb 2011 15:28:05 +0300 Alexander Petukhov Alexander.Petukhov@mail.ru wrote:

5. Other language bindings - don't really think it can increase

plugins quality dramatically, there can be problems in any language that you have to solve in order to make your code work correctly.

I agree. It will might also split up resources on core side to support maybe 5 API as even you do use some kind of automatically binding it will not working out of the box in every case.

Cheers, Frank

On 12 March 2011 11:35, Matthew Brush matthewbrush@gmail.com wrote:

On 03/11/11 15:24, Frank Lanitz wrote:

...

On Wed, 23 Feb 2011 15:28:05 +0300 Alexander PetukhovAlexander.Petukhov@mail.ru  wrote:

...

5. Other language bindings - don't really think it can increase

plugins quality dramatically, there can be problems in any language that you have to solve in order to make your code work correctly.

I agree. It will might also split up resources on core side to support maybe 5 API as even you do use some kind of automatically binding it will not working out of the box in every case.

If the core API were more binding friendly, for example using standard GObject conventions, a HUGE portion of the work doing bindings would be completely automated for quite a number of languages, without any need to customize the core for them.  The things that don't work out of the box would be fixed in the binding itself as is common with other bindings of things.  This is the whole purpose of things like GObject introspection (to make GObject bindings automatic and natural to that language).

As an example, the Vala bindings are being hacked to death to make them more like Vala-ish (make that GObject-ish), when this would be a basically fully automated process if the API was already GObject-ish.  Same with Genie, Python, and probably others with GObject support like Perl, Ruby, Scheme, C++, and so on.

Even if it didn't lead to improved plugin quality, which it would at least for languages using VMs/interpreters, it would create a lot of new functionality for Geany by having much more participation from a wider range of programmers.  Just hanging around on the ML and IRC I've heard many times Geany users saying they would like to make a plugin but they don't know C, they are experts on some other language.  One of the big benefits of a plugin system is making it easy for users to extend the program, and IMO making the users learn and code properly in C does not (necessarily) accomplish that goal.

The barrier to entry to writing Geany plugins currently is such that a very small portion of the users (programmers) are able to extend the software at all, and those that fake it in C will end up with nasty mess of buggy code (like me :).

My 0.02$ CAD

Matthew Brush (codebrainz)

Hi Matthew,

I wasn't party to the original design of the Geany plugin API/ABI so I don't have any investment in it.

I can see why it was done the way it is, it has several potential advantages in terms of reducing plugin re-compilation requirements, although it loses some of that by being very conservative (but safe) in terms of its run time acceptance of older plugins. But as you point out it isn't easy to bind to.

As we have discussed before, replacing the interface isn't acceptable, but that doesn't mean that a second interface that is automatically GObjectable, SWIGable introspectable etc.can't be added.

Over time existing plugins can then be migrated and the old interface removed.

But someone has to do it (tm) and of course patches are welcome (c)...

Cheers Lex

$0.02AUD worth about the same :-)

---

Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel

On Wed, 23 Feb 2011 11:10:02 +0100 Thomas Martitz thomas.martitz@student.htw-berlin.de wrote:

Am 23.02.2011 04:01, schrieb Lex Trotman:

...

Of course on the other hand because the maintainers are volunteers with limited time this process slows changes, but IMHO this is necessary. Having more maintainers for a piece of code is of course the solution, but as we are finding, its hard to actually do for a small project. Even for core there was a deafening silence when I recently suggested that more people volunteer to be maintainers.

I don't think that silence meant anything other than the people that can decide haven't read it yet. But yea, I would welcome a discussion about this in the core. There's great potential contributors on this list, but they're all sitting on their patches and nothing moves forward.

Including patches always means to review them. Also granting access is a delicate thing and adding just everybody to accesslist is bad idea in terms of e.g. quality as everybody might can understand. Unfortunately as none of us is paid for doing Geany development and stuff around its hard to just read the tons of mails after a 10h or more working day. Also there is some life around.

Cheers, Frank

On Tue, 08 Mar 2011 11:06:16 +0100, Frank wrote:

Am 23.02.2011 01:10, schrieb Matthew Brush:

...

For first thing, maybe we could enforce use/passing of those tools mentioned and these before adding to release, examples: http://www.splint.org/ http://valgrind.org/info/tools.html (suppression for GTK - http://people.gnome.org/~johan/gtk.suppression) http://www.gnu.org/software/indent/ (just for making coding styles more consistent between plugins) http://check.sourceforge.net/ or http://cutest.sourceforge.net/ or http://cunit.sourceforge.net/ Perhaps some or all of these could be automated.

I like that idea. Can someone of you build up a howto on how to use it? I did try valgrind in past and wished for some advice ;)

One this is done we can think of automatic tests with some of this tools.

I, and obviously, Colomban as well, though indepdent from each other :D, recently played[1,2] with cppcheck. A small tool for static code analysis which actually found a few things in the geany-plugins repository.

As I'm currently reworking the system to create the nightly builds, we could integrate such checks into the nightly builds, maybe run cppcheck on the sources after the builds and present the results somewhere on nightly.geany.org.

Just an idea.

[1] http://lists.uvena.de/geany-devel/2011-February/003885.html [2] http://lists.uvena.de/geany-devel/2011-February/003981.html

Regards, Enrico

Le 08/03/2011 19:58, Enrico Tröger a écrit :

On Tue, 08 Mar 2011 11:06:16 +0100, Frank wrote:

...

Am 23.02.2011 01:10, schrieb Matthew Brush:

...

For first thing, maybe we could enforce use/passing of those tools mentioned and these before adding to release, examples: http://www.splint.org/ http://valgrind.org/info/tools.html (suppression for GTK - http://people.gnome.org/~johan/gtk.suppression) http://www.gnu.org/software/indent/ (just for making coding styles more consistent between plugins) http://check.sourceforge.net/ or http://cutest.sourceforge.net/ or http://cunit.sourceforge.net/ Perhaps some or all of these could be automated.

I don't suggest (yet) to use splint, because I haven't found it useful, it reported ways too much things (and some was false positive) to be really usable. However, I probably don't know how to configure it (and haven't the time to find out yet), so if somebody with experience with it can provide some hints, maybe we can add this too.

For Valgrind however, I doubt we can do anything automatically, because it's a runtime checker, and its output must generally be read with some care. Writing some sort of guidelines and howto is probably the way to go.

...

I like that idea. Can someone of you build up a howto on how to use it? I did try valgrind in past and wished for some advice ;)

One this is done we can think of automatic tests with some of this tools.

I, and obviously, Colomban as well, though indepdent from each other :D, recently played[1,2] with cppcheck. A small tool for static code analysis which actually found a few things in the geany-plugins repository.

As I'm currently reworking the system to create the nightly builds, we could integrate such checks into the nightly builds, maybe run cppcheck on the sources after the builds and present the results somewhere on nightly.geany.org.

I think it's a good idea.

I did a few checks, and this is what I suggest: 1) run cppcheck on `make check` and abort if it detects an error; 2) enable by default (though in a portable manner) some compiler flags such as -Werror-implicit-function-declaration [1] [2]

Maybe some other tests might be good, but I think this is a start.

I'm working on implementing this for Autotools, maybe I can take a look at Waf too (but Enrico would love to do this :D).

Thoughts?

Cheers, Colomban

[1] and maybe some other flags, but implicit function declaration is a very crash-prone bug, and I can't think of (and never saw) a case where this would break legitimate code.

[2] for example, debugger plugin currently have some implicit function declarations -- Alexander, could you fix this?

Hi Colomban,

Agree with most, comment on the rest below.

On 9 March 2011 14:00, Colomban Wendling lists.ban@herbesfolles.org wrote:

Le 08/03/2011 22:29, Colomban Wendling a écrit :

...

Le 08/03/2011 19:58, Enrico Tröger a écrit :

...

On Tue, 08 Mar 2011 11:06:16 +0100, Frank wrote:

...

Am 23.02.2011 01:10, schrieb Matthew Brush:

...

For first thing, maybe we could enforce use/passing of those tools mentioned and these before adding to release, examples: http://www.splint.org/ http://valgrind.org/info/tools.html (suppression for GTK - http://people.gnome.org/~johan/gtk.suppression) http://www.gnu.org/software/indent/ (just for making coding styles more consistent between plugins) http://check.sourceforge.net/ or http://cutest.sourceforge.net/ or http://cunit.sourceforge.net/ Perhaps some or all of these could be automated.

I don't suggest (yet) to use splint, because I haven't found it useful, it reported ways too much things (and some was false positive) to be really usable. However, I probably don't know how to configure it (and haven't the time to find out yet), so if somebody with experience with it can provide some hints, maybe we can add this too.

For Valgrind however, I doubt we can do anything automatically, because it's a runtime checker, and its output must generally be read with some care. Writing some sort of guidelines and howto is probably the way to go.

...

...

I like that idea. Can someone of you build up a howto on how to use it? I did try valgrind in past and wished for some advice ;)

One this is done we can think of automatic tests with some of this tools.

I, and obviously, Colomban as well, though indepdent from each other :D, recently played[1,2] with cppcheck. A small tool for static code analysis which actually found a few things in the geany-plugins repository.

As I'm currently reworking the system to create the nightly builds, we could integrate such checks into the nightly builds, maybe run cppcheck on the sources after the builds and present the results somewhere on nightly.geany.org.

I think it's a good idea.

I did a few checks, and this is what I suggest:

1. run cppcheck on `make check` and abort if it detects an error;
2. enable by default (though in a portable manner) some compiler flags

such as -Werror-implicit-function-declaration [1] [2]

After a little more thinking and testing, I suggest:

• -Werror=implicit-function-declaration (see above);
• -Werror=pointer-arith, to avoid portability issues with untyped

pointer arithmetic;

• -Wundef because preprocessor tests should rely on defined variables or

explicitly use ifdef;

• -Wshadow because shadowing symbols is a Bad Practice (and maybe it is

even non-standard, not quite sure however);

Agree for shadow of locals and parameters, agree for globals but only because all globals should be prefixed by a module prefix like the Geany_ symbols are. Is there any way to check this?

• -Waggregate-return because returning aggregate values is no good for

performances and it's not a Good Practice in C;

Why isn't it good? Performance should be as good if not better than returning multiple parameters via pointers and then assembling the aggregate in the caller. Why isn't it Good Practice?? (your caps :-) Its been standard ever since someone invented allocating the return variable on the stack prior to the call, so it can be referenced relative to the frame pointer.

• -Wwrite-strings because using a string literal as a modifiable value

may lead to crashes, and anyway good code should probably assume it's constant.

I thing all is perfectly fine but maybe -Wwrite-strings. What -Wwrite-strings does is set all string literals ("foobar") constant (const char*). It is a good practice to assume string literals are constant, but however it is not necessarily wrong to assign it to a non-constant variable (if one actually don't modify the value), so it may have false positive. And actually it probably has: it adds lots of warnings in some plugins, and though I haven't checked, I doubt all are true errors. I think it'd be a good thing if no plugin would emit a warning using this, but maybe it's too soon for this, or maybe we don't want this at all, not sure. So, do you think it is or not a good idea to use it?

I'd agree that "mostly" strings should be treated as const, but sometimes they are a template in which code replaces characters with values. I havn't looked at the plugins but I'd guess that most of the warnings are from initialising char * instead of const char *. Us C++ programmers get used to using const char * for string literals. For the few that really do mean to be written to, initialising a char[] works fine.

And finally, I though of -Wstrict-prototypes, -Wmissing-prototypes and -Wmissing-declarations [1], but...

• -Wstrict-prototypes even emits a warning in a GTK+ header and one in

Scintilla.h;

• -Wmissing-prototypes (and -Wmissing-declarations) emits a warning for

plugin_init() and friends (though I thought it was fixed already?).

...So we probably don't want them by default. Moreover, the problem reported by -Wmissing-prototypes is not really important for preventing crashes, it's mostly a "beauty" thing.

[1] not sure of the difference with -Wmissing-prototypes...

IIUC prototypes is C, declarations is C++ because it allows some situations that C++ explicitly requires direct definition eg in templates or inlines.

Cheers Lex

...

Maybe some other tests might be good, but I think this is a start.

I'm working on implementing this for Autotools, maybe I can take a look at Waf too (but Enrico would love to do this :D).

Thoughts?

Cheers, Colomban _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel

On 10 March 2011 00:45, Colomban Wendling lists.ban@herbesfolles.org wrote:

Le 09/03/2011 05:31, Lex Trotman a écrit :

...

Hi Colomban,

Agree with most, comment on the rest below.

On 9 March 2011 14:00, Colomban Wendling lists.ban@herbesfolles.org wrote:

...

Le 08/03/2011 22:29, Colomban Wendling a écrit :

...

Le 08/03/2011 19:58, Enrico Tröger a écrit :

...

On Tue, 08 Mar 2011 11:06:16 +0100, Frank wrote:

...

Am 23.02.2011 01:10, schrieb Matthew Brush: > For first thing, maybe we could enforce use/passing of those tools > mentioned and these before adding to release, examples: > http://www.splint.org/ > http://valgrind.org/info/tools.html > (suppression for GTK - > http://people.gnome.org/~johan/gtk.suppression) > http://www.gnu.org/software/indent/ (just for making coding styles > more consistent between plugins) http://check.sourceforge.net/ or > http://cutest.sourceforge.net/ or http://cunit.sourceforge.net/ > Perhaps some or all of these could be automated.

I don't suggest (yet) to use splint, because I haven't found it useful, it reported ways too much things (and some was false positive) to be really usable. However, I probably don't know how to configure it (and haven't the time to find out yet), so if somebody with experience with it can provide some hints, maybe we can add this too.

For Valgrind however, I doubt we can do anything automatically, because it's a runtime checker, and its output must generally be read with some care. Writing some sort of guidelines and howto is probably the way to go.

...

...

I like that idea. Can someone of you build up a howto on how to use it? I did try valgrind in past and wished for some advice ;)

One this is done we can think of automatic tests with some of this tools.

I, and obviously, Colomban as well, though indepdent from each other :D, recently played[1,2] with cppcheck. A small tool for static code analysis which actually found a few things in the geany-plugins repository.

As I'm currently reworking the system to create the nightly builds, we could integrate such checks into the nightly builds, maybe run cppcheck on the sources after the builds and present the results somewhere on nightly.geany.org.

I think it's a good idea.

I did a few checks, and this is what I suggest:

1. run cppcheck on `make check` and abort if it detects an error;
2. enable by default (though in a portable manner) some compiler flags

such as -Werror-implicit-function-declaration [1] [2]

After a little more thinking and testing, I suggest:

• -Werror=implicit-function-declaration (see above);
• -Werror=pointer-arith, to avoid portability issues with untyped

pointer arithmetic;

• -Wundef because preprocessor tests should rely on defined variables or

explicitly use ifdef;

• -Wshadow because shadowing symbols is a Bad Practice (and maybe it is

even non-standard, not quite sure however);

Agree for shadow of locals and parameters, agree for globals but only because all globals should be prefixed by a module prefix like the Geany_ symbols are.  Is there any way to check this?

Not that I know, unfortunately. However maybe a dumb sed/grep might do the trick.

...

...

• -Waggregate-return because returning aggregate values is no good for

performances and it's not a Good Practice in C;

Why isn't it good?  Performance should be as good if not better than returning multiple parameters via pointers and then assembling the aggregate in the caller.  Why isn't it Good Practice?? (your caps :-) Its been standard ever since someone invented allocating the return variable on the stack prior to the call, so it can be referenced relative to the frame pointer.

Well... I must admit I'm not really pro on the subject, but AFAIK the main problem is ABI, because since the return value will not necessarily fit in registers the compiler is likely to generate a dummy parameter to the function (as you might have done yourself), but this implicitly, so changing the "natural" ABI of the function.

Functions that return too much to fit in the registers should use the stdcall ABI which defines an extra reference parameter for the aggregate. (Or rather the compiler should use it). There is no such thing as a "natural" ABI that is corrupted by aggregate returns, there are ABIs defined by Intel and other CPU makers for all the required call/return patterns. Yes its slower than returning something in a register, but if you have got to return an aggregate value thats too big you have to return it. Might as well let the compiler do the heavy lifting rather than having to declare a temporary and pass a pointer to it as an explicit parameter or otherwise return the components and assemble the aggregate in the caller.

Back in the dim distant past (1970s) when function prototypes were as rare as hens teeth such returns were a drama since they required a prototype. I don't see why we should continue this restriction as a hangover from the dinosaurs and the voracious EGCS compiler raptor :-).

Not that the restriction would have much impact, a survey found about 95% of C code returned void, some integer type or some pointer type, probably because thats the way the libraries are written and everyone blindly uses the same style. But why impose an arbitrary restriction on standard compliant code? Someone who happened to want to use a more pure functional coding style might even produce better code than the side effect ridden normal C style. (At least thats what the smart PHDs seem to indicate).

Any'ow I'll climb down off me soapbox now.

However it's true that since we don't speak about libraries, we don't mind about that.

As I said above, there is a defined call ABI for functions returning aggregates, all such functions should use it, libraries or not.

Another point seems to be that some ooold compilers used not to support aggregate return, though once again we probably don't mind.

Well then they are not standard compliant, so I agree they don't matter :-)

OTOH, I don't know of any good (or not BTW) code using aggregates as return values; maybe there's other reasons?

Habit, see above :-)

Cheers Lex

PS IIUC most current compilers will compile

struct big_aggregate a = f();

to pass a as the return parameter directly so no copy happens. That used to be another objection to returning aggregates.

However, I don't really mind about this one, since even if I used to think it's a performance issue, it's not a really important point. We're trying to make code stronger, not necessarily faster :D

...

...

• -Wwrite-strings because using a string literal as a modifiable value

may lead to crashes, and anyway good code should probably assume it's constant.

I thing all is perfectly fine but maybe -Wwrite-strings. What -Wwrite-strings does is set all string literals ("foobar") constant (const char*). It is a good practice to assume string literals are constant, but however it is not necessarily wrong to assign it to a non-constant variable (if one actually don't modify the value), so it may have false positive. And actually it probably has: it adds lots of warnings in some plugins, and though I haven't checked, I doubt all are true errors. I think it'd be a good thing if no plugin would emit a warning using this, but maybe it's too soon for this, or maybe we don't want this at all, not sure. So, do you think it is or not a good idea to use it?

I'd agree that "mostly" strings should be treated as const, but sometimes they are a template in which code replaces characters with values.  I havn't looked at the plugins but I'd guess that most of the warnings are from initialising char * instead of const char *.  Us C++ programmers get used to using const char * for string literals.  For the few that really do mean to be written to, initialising a char[] works fine.

Yes, none of the warnings that this flag would trigger are either hard or wrong to fix. I personally think it's worth using it, just wanted to see if concerned people would care to fix these.

Cheers, Colomban

...

...

And finally, I though of -Wstrict-prototypes, -Wmissing-prototypes and -Wmissing-declarations [1], but...

• -Wstrict-prototypes even emits a warning in a GTK+ header and one in

Scintilla.h;

• -Wmissing-prototypes (and -Wmissing-declarations) emits a warning for

plugin_init() and friends (though I thought it was fixed already?).

...So we probably don't want them by default. Moreover, the problem reported by -Wmissing-prototypes is not really important for preventing crashes, it's mostly a "beauty" thing.

[1] not sure of the difference with -Wmissing-prototypes...

IIUC prototypes is C, declarations is C++ because it allows some situations that C++ explicitly requires direct definition eg in templates or inlines.

Cheers Lex

...

...

Maybe some other tests might be good, but I think this is a start.

I'm working on implementing this for Autotools, maybe I can take a look at Waf too (but Enrico would love to do this :D).

Thoughts?

Cheers, Colomban _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel

On 10 March 2011 11:57, Thomas Martitz thomas.martitz@student.htw-berlin.de wrote:

On 10.03.2011 01:23, Lex Trotman wrote:

...

On 10 March 2011 00:45, Colomban Wendlinglists.ban@herbesfolles.org  wrote:

...

Le 09/03/2011 05:31, Lex Trotman a écrit :

...

Hi Colomban,

Agree with most, comment on the rest below.

On 9 March 2011 14:00, Colomban Wendlinglists.ban@herbesfolles.org  wrote:

...

Le 08/03/2011 22:29, Colomban Wendling a écrit :

...

Le 08/03/2011 19:58, Enrico Tröger a écrit : > > On Tue, 08 Mar 2011 11:06:16 +0100, Frank wrote: > >> Am 23.02.2011 01:10, schrieb Matthew Brush: >>> >>> For first thing, maybe we could enforce use/passing of those tools >>> mentioned and these before adding to release, examples: >>> http://www.splint.org/ >>> http://valgrind.org/info/tools.html >>> (suppression for GTK - >>> http://people.gnome.org/~johan/gtk.suppression) >>> http://www.gnu.org/software/indent/ (just for making coding styles >>> more consistent between plugins) http://check.sourceforge.net/ or >>> http://cutest.sourceforge.net/ or http://cunit.sourceforge.net/ >>> Perhaps some or all of these could be automated.

I don't suggest (yet) to use splint, because I haven't found it useful, it reported ways too much things (and some was false positive) to be really usable. However, I probably don't know how to configure it (and haven't the time to find out yet), so if somebody with experience with it can provide some hints, maybe we can add this too.

For Valgrind however, I doubt we can do anything automatically, because it's a runtime checker, and its output must generally be read with some care. Writing some sort of guidelines and howto is probably the way to go.

>> I like that idea. Can someone of you build up a howto on how to use >> it? >> I did try valgrind in past and wished for some advice ;) >> >> One this is done we can think of automatic tests with some of this >> tools. > > I, and obviously, Colomban as well, though indepdent from each other > :D, > recently played[1,2] with cppcheck. A small tool for static code > analysis which actually found a few things in the geany-plugins > repository. > > As I'm currently reworking the system to create the nightly builds, > we > could integrate such checks into the nightly builds, maybe run > cppcheck > on the sources after the builds and present the results somewhere on > nightly.geany.org.

I think it's a good idea.

I did a few checks, and this is what I suggest:

1. run cppcheck on `make check` and abort if it detects an error;
2. enable by default (though in a portable manner) some compiler flags

such as -Werror-implicit-function-declaration [1] [2]

After a little more thinking and testing, I suggest:

• -Werror=implicit-function-declaration (see above);
• -Werror=pointer-arith, to avoid portability issues with untyped

pointer arithmetic;

• -Wundef because preprocessor tests should rely on defined variables

or explicitly use ifdef;

• -Wshadow because shadowing symbols is a Bad Practice (and maybe it is

even non-standard, not quite sure however);

Agree for shadow of locals and parameters, agree for globals but only because all globals should be prefixed by a module prefix like the Geany_ symbols are.  Is there any way to check this?

Not that I know, unfortunately. However maybe a dumb sed/grep might do the trick.

...

...

• -Waggregate-return because returning aggregate values is no good for

performances and it's not a Good Practice in C;

Why isn't it good?  Performance should be as good if not better than returning multiple parameters via pointers and then assembling the aggregate in the caller.  Why isn't it Good Practice?? (your caps :-) Its been standard ever since someone invented allocating the return variable on the stack prior to the call, so it can be referenced relative to the frame pointer.

Well... I must admit I'm not really pro on the subject, but AFAIK the main problem is ABI, because since the return value will not necessarily fit in registers the compiler is likely to generate a dummy parameter to the function (as you might have done yourself), but this implicitly, so changing the "natural" ABI of the function.

Functions that return too much to fit in the registers should use the stdcall ABI which defines an extra reference parameter for the aggregate. (Or rather the compiler should use it).  There is no such thing as a "natural" ABI that is corrupted by aggregate returns, there are ABIs defined by Intel and other CPU makers for all the required call/return patterns.  Yes its slower than returning something in a register, but if you have got to return an aggregate value thats too big you have to return it.  Might as well let the compiler do the heavy lifting rather than having to declare a temporary and pass a pointer to it as an explicit parameter or otherwise return the components and assemble the aggregate in the caller.

Returning an aggregate is exactly the same as manually allocating the aggregate and passing a pointer to the function (in order to let it fill the aggregate) on most if not all ABIs. Without any difference in performance.

Exactly, summarised beautifully.

Of course the latter has the advantage that you can additionally return a failure indicating value (-1 or NULL) so this is why you generally don't see functions returning aggregates.

Yes, its the habit created by the standard libraries, and they did it that way because C doesn't have an out-of-band signaling mechanism to handle errors. So its all K&Rs fault... :-)

Cheers Lex

Best regards. _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel

Le 10/03/2011 01:23, Lex Trotman a écrit :

On 10 March 2011 00:45, Colomban Wendling lists.ban@herbesfolles.org wrote:

...

Le 09/03/2011 05:31, Lex Trotman a écrit :

...

Hi Colomban,

Agree with most, comment on the rest below.

On 9 March 2011 14:00, Colomban Wendling lists.ban@herbesfolles.org wrote:

...

Le 08/03/2011 22:29, Colomban Wendling a écrit :

...

Le 08/03/2011 19:58, Enrico Tröger a écrit :

...

On Tue, 08 Mar 2011 11:06:16 +0100, Frank wrote:

> Am 23.02.2011 01:10, schrieb Matthew Brush: >> For first thing, maybe we could enforce use/passing of those tools >> mentioned and these before adding to release, examples: >> http://www.splint.org/ >> http://valgrind.org/info/tools.html >> (suppression for GTK - >> http://people.gnome.org/~johan/gtk.suppression) >> http://www.gnu.org/software/indent/ (just for making coding styles >> more consistent between plugins) http://check.sourceforge.net/ or >> http://cutest.sourceforge.net/ or http://cunit.sourceforge.net/ >> Perhaps some or all of these could be automated.

I don't suggest (yet) to use splint, because I haven't found it useful, it reported ways too much things (and some was false positive) to be really usable. However, I probably don't know how to configure it (and haven't the time to find out yet), so if somebody with experience with it can provide some hints, maybe we can add this too.

For Valgrind however, I doubt we can do anything automatically, because it's a runtime checker, and its output must generally be read with some care. Writing some sort of guidelines and howto is probably the way to go.

...

> > I like that idea. Can someone of you build up a howto on how to use it? > I did try valgrind in past and wished for some advice ;) > > One this is done we can think of automatic tests with some of this > tools.

I, and obviously, Colomban as well, though indepdent from each other :D, recently played[1,2] with cppcheck. A small tool for static code analysis which actually found a few things in the geany-plugins repository.

As I'm currently reworking the system to create the nightly builds, we could integrate such checks into the nightly builds, maybe run cppcheck on the sources after the builds and present the results somewhere on nightly.geany.org.

I think it's a good idea.

I did a few checks, and this is what I suggest:

1. run cppcheck on `make check` and abort if it detects an error;
2. enable by default (though in a portable manner) some compiler flags

such as -Werror-implicit-function-declaration [1] [2]

After a little more thinking and testing, I suggest:

• -Werror=implicit-function-declaration (see above);
• -Werror=pointer-arith, to avoid portability issues with untyped

pointer arithmetic;

• -Wundef because preprocessor tests should rely on defined variables or

explicitly use ifdef;

• -Wshadow because shadowing symbols is a Bad Practice (and maybe it is

even non-standard, not quite sure however);

Agree for shadow of locals and parameters, agree for globals but only because all globals should be prefixed by a module prefix like the Geany_ symbols are. Is there any way to check this?

Not that I know, unfortunately. However maybe a dumb sed/grep might do the trick.

...

...

• -Waggregate-return because returning aggregate values is no good for

performances and it's not a Good Practice in C;

Why isn't it good? Performance should be as good if not better than returning multiple parameters via pointers and then assembling the aggregate in the caller. Why isn't it Good Practice?? (your caps :-) Its been standard ever since someone invented allocating the return variable on the stack prior to the call, so it can be referenced relative to the frame pointer.

Well... I must admit I'm not really pro on the subject, but AFAIK the main problem is ABI, because since the return value will not necessarily fit in registers the compiler is likely to generate a dummy parameter to the function (as you might have done yourself), but this implicitly, so changing the "natural" ABI of the function.

Functions that return too much to fit in the registers should use the stdcall ABI which defines an extra reference parameter for the aggregate. (Or rather the compiler should use it). There is no such thing as a "natural" ABI that is corrupted by aggregate returns, there are ABIs defined by Intel and other CPU makers for all the required call/return patterns. Yes its slower than returning something in a register, but if you have got to return an aggregate value thats too big you have to return it. Might as well let the compiler do the heavy lifting rather than having to declare a temporary and pass a pointer to it as an explicit parameter or otherwise return the components and assemble the aggregate in the caller.

Back in the dim distant past (1970s) when function prototypes were as rare as hens teeth such returns were a drama since they required a prototype. I don't see why we should continue this restriction as a hangover from the dinosaurs and the voracious EGCS compiler raptor :-).

Not that the restriction would have much impact, a survey found about 95% of C code returned void, some integer type or some pointer type, probably because thats the way the libraries are written and everyone blindly uses the same style. But why impose an arbitrary restriction on standard compliant code? Someone who happened to want to use a more pure functional coding style might even produce better code than the side effect ridden normal C style. (At least thats what the smart PHDs seem to indicate).

Any'ow I'll climb down off me soapbox now.

So maybe I was just plain wrong and should update my bias :) I actually don't know much about this -- and I should blame myself to speak about it then.

Cheers, Colomban

...

However it's true that since we don't speak about libraries, we don't mind about that.

As I said above, there is a defined call ABI for functions returning aggregates, all such functions should use it, libraries or not.

...

Another point seems to be that some ooold compilers used not to support aggregate return, though once again we probably don't mind.

Well then they are not standard compliant, so I agree they don't matter :-)

...

OTOH, I don't know of any good (or not BTW) code using aggregates as return values; maybe there's other reasons?

Habit, see above :-)

Cheers Lex

PS IIUC most current compilers will compile

struct big_aggregate a = f();

to pass a as the return parameter directly so no copy happens. That used to be another objection to returning aggregates.

Le 08/03/2011 22:29, Colomban Wendling a écrit :

Le 08/03/2011 19:58, Enrico Tröger a écrit :

...

On Tue, 08 Mar 2011 11:06:16 +0100, Frank wrote:

...

I like that idea. Can someone of you build up a howto on how to use it? I did try valgrind in past and wished for some advice ;)

One this is done we can think of automatic tests with some of this tools.

I, and obviously, Colomban as well, though indepdent from each other :D, recently played[1,2] with cppcheck. A small tool for static code analysis which actually found a few things in the geany-plugins repository.

As I'm currently reworking the system to create the nightly builds, we could integrate such checks into the nightly builds, maybe run cppcheck on the sources after the builds and present the results somewhere on nightly.geany.org.

I think it's a good idea.

I did a few checks, and this is what I suggest:

1. run cppcheck on `make check` and abort if it detects an error;
2. enable by default (though in a portable manner) some compiler flags

such as -Werror-implicit-function-declaration [1] [2]

Maybe some other tests might be good, but I think this is a start.

I'd like to commit this to the Autotools build system:

1) run cppcheck on `make check`; 2) enable by default, if compiler understands them, the following warnings (discussed in other mails of this thread): * -Werror=implicit-function-declaration * -Werror=pointer-arith * -Wundef * -Wshadow * -Wwrite-strings

There are currently 2 problems that would prevents the tests to pass: 1) The debugger plugin don't compile with -Werror=implicit-function-declaration (this should be fixed -- Alexander, could you fix them please?); 2) cppcheck reports an error on geanylatex plugin; but I know Frank already fixed this and so has probably only to import the fix in the geany-plugins copy.

1 is really problematic since it require one to disable the debugger plugin to be able to compiler the others, 2 is only problematic when running `make check` so not so important in the meantime Frank fixes the problem.

Do you have objections, comment or whatever?

Cheers, Colomban

...

Maybe some other tests might be good, but I think this is a start.

I'd like to commit this to the Autotools build system:

1. run cppcheck on `make check`;
2. enable by default, if compiler understands them, the following

warnings (discussed in other mails of this thread):  * -Werror=implicit-function-declaration  * -Werror=pointer-arith  * -Wundef  * -Wshadow  * -Wwrite-strings

Good start.

There are currently 2 problems that would prevents the tests to pass:

1. The debugger plugin don't compile with

-Werror=implicit-function-declaration (this should be fixed -- Alexander, could you fix them please?); 2) cppcheck reports an error on geanylatex plugin; but I know Frank already fixed this and so has probably only to import the fix in the geany-plugins copy.

1 is really problematic since it require one to disable the debugger plugin to be able to compiler the others,

Why do we have to disable the dubugger, sure it gives warnings, but for nightly builds and SVN builds thats ok, the disabling only comes at release time.

Cheers Lex

2 is only problematic when

running `make check` so not so important in the meantime Frank fixes the problem.

Do you have objections, comment or whatever?

Cheers, Colomban _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel

On 12 March 2011 11:23, Colomban Wendling lists.ban@herbesfolles.org wrote:

Le 12/03/2011 01:18, Lex Trotman a écrit :

...

...

...

Maybe some other tests might be good, but I think this is a start.

I'd like to commit this to the Autotools build system:

1. run cppcheck on `make check`;
2. enable by default, if compiler understands them, the following

warnings (discussed in other mails of this thread):  * -Werror=implicit-function-declaration  * -Werror=pointer-arith  * -Wundef  * -Wshadow  * -Wwrite-strings

Good start.

Feel free to suggest more :)

...

...

There are currently 2 problems that would prevents the tests to pass:

1. The debugger plugin don't compile with

-Werror=implicit-function-declaration (this should be fixed -- Alexander, could you fix them please?); 2) cppcheck reports an error on geanylatex plugin; but I know Frank already fixed this and so has probably only to import the fix in the geany-plugins copy.

1 is really problematic since it require one to disable the debugger plugin to be able to compiler the others,

Why do we have to disable the dubugger, sure it gives warnings, but for nightly builds and SVN builds thats ok, the disabling only comes at release time.

Since I set -W*error=*implicit-function-declaration, the implicit function declaration warnings are treated as errors and then aborts the build (unless one uses make -k of course).

We could downgrade this to a warning, but I think this is a problem important enough to trigger an error.

The general consensus seemed to be to not disable plugins from the nightly builds or SVN just because they fail some tests, so these will have to all be warnings.

There still seems to be some discussion on how to handle plugins that don't compile cleanly, or have documentation at release time.

Cheers Lex

Cheers, Colomban

On 12 March 2011 11:44, Colomban Wendling lists.ban@herbesfolles.org wrote:

Le 12/03/2011 01:32, Lex Trotman a écrit :

...

On 12 March 2011 11:23, Colomban Wendling lists.ban@herbesfolles.org wrote:

...

Le 12/03/2011 01:18, Lex Trotman a écrit :

...

...

...

Maybe some other tests might be good, but I think this is a start.

I'd like to commit this to the Autotools build system:

1. run cppcheck on `make check`;
2. enable by default, if compiler understands them, the following

warnings (discussed in other mails of this thread):  * -Werror=implicit-function-declaration  * -Werror=pointer-arith  * -Wundef  * -Wshadow  * -Wwrite-strings

Good start.

Feel free to suggest more :)

...

...

There are currently 2 problems that would prevents the tests to pass:

1. The debugger plugin don't compile with

-Werror=implicit-function-declaration (this should be fixed -- Alexander, could you fix them please?); 2) cppcheck reports an error on geanylatex plugin; but I know Frank already fixed this and so has probably only to import the fix in the geany-plugins copy.

1 is really problematic since it require one to disable the debugger plugin to be able to compiler the others,

Why do we have to disable the dubugger, sure it gives warnings, but for nightly builds and SVN builds thats ok, the disabling only comes at release time.

Since I set -W*error=*implicit-function-declaration, the implicit function declaration warnings are treated as errors and then aborts the build (unless one uses make -k of course).

We could downgrade this to a warning, but I think this is a problem important enough to trigger an error.

The general consensus seemed to be to not disable plugins from the nightly builds or SVN just because they fail some tests, so these will have to all be warnings.

It's a bit more complicated IMO: if these warnings are on by default in everyone's build, a code failing with them would just be as invalid as an invalid C code (e.g. breaks everyone's build, and isn't acceptable at all).

Well on the development code base warnings should not break the build, if they do thats another problem :-) In fact even errors should also not break the whole build, just the specific plugin.

The problem here is that there is currently a plugin that can't be compiled with them, so enabling them would mean disabling the plugin that used to build.

Maybe the solution is to wait for Alexander to fix these problems, and then enable the "errors".

But the next patch commit on any plugin could fail one of the checks, so then the whole dev build fails again, thats no good, its got to still build with warnings, its a development build after all.

Cheers Lex

Cheers, Colomban

...

There still seems to be some discussion on how to handle plugins that don't compile cleanly, or have documentation at release time.

Cheers Lex

...

Cheers, Colomban

...

...

...

The general consensus seemed to be to not disable plugins from the nightly builds or SVN just because they fail some tests, so these will have to all be warnings.

It's a bit more complicated IMO: if these warnings are on by default in everyone's build, a code failing with them would just be as invalid as an invalid C code (e.g. breaks everyone's build, and isn't acceptable at all).

Well on the development code base warnings should not break the build, if they do thats another problem :-)

...and invalid C code should neither, but it does :(

Hmmm, that IS a problem, do we know why? Is make -k sufficient to fix it? (Sorry I can't try building things on this machine so I can only ask the question)

What I mean is that it is acceptable IMO to consider some "mistakes" as "invalid syntax".

Maaaybe, sort of see your point, but not really convinced that uprating warnings to errors is a good idea on the dev codebase, it stops people trying and testing things.

...

In fact even errors should also not break the whole build, just the specific plugin.

agreed. But that's a somewhat different point I guess... maybe just tell people to use `make -k` ^^

Yep.

...

...

The problem here is that there is currently a plugin that can't be compiled with them, so enabling them would mean disabling the plugin that used to build.

Maybe the solution is to wait for Alexander to fix these problems, and then enable the "errors".

But the next patch commit on any plugin could fail one of the checks, so then the whole dev build fails again, thats no good, its got to still build with warnings, its a development build after all.

but again, what if I add

hello guys!

in the middle of the code? Or even more realistic (someone might even understand... :D)

if some test { foo_bar(); }

In this case, what would you do? Blame the programmer most probably. And you'd be right IMO :)

Of course it should fail the plugin, but as above, not the whole build.

And I hope we use "blame" in the non-emotive sense of "identify the cause of the problem" :-)

Cheers Lex

Cheers, Colomban

On 12 March 2011 12:53, Colomban Wendling lists.ban@herbesfolles.org wrote:

Le 12/03/2011 02:37, Lex Trotman a écrit :

...

...

...

...

...

The general consensus seemed to be to not disable plugins from the nightly builds or SVN just because they fail some tests, so these will have to all be warnings.

It's a bit more complicated IMO: if these warnings are on by default in everyone's build, a code failing with them would just be as invalid as an invalid C code (e.g. breaks everyone's build, and isn't acceptable at all).

Well on the development code base warnings should not break the build, if they do thats another problem :-)

...and invalid C code should neither, but it does :(

Hmmm, that IS a problem, do we know why? Is make -k sufficient to fix it?  (Sorry I can't try building things on this machine so I can only ask the question)

...

What I mean is that it is acceptable IMO to consider some "mistakes" as "invalid syntax".

Maaaybe, sort of see your point, but not really convinced that uprating warnings to errors is a good idea on the dev codebase, it stops people trying and testing things.

Unfortunately, believe me that non-fatal warnings are use to be ignored by unexperienced programmers, believing that if their code compile it is then OK. And I don't see why a warning upgraded to an error on every build would be worst than a syntactical problem (as I described above previously)? In a typical situation, the developer who writes the plugin should get the warning (well, the error), see his plugin don't build, care (hopefully :D), and then fix it directly even before committing and then before anybody else could face the problem. Don't you think?

I agree that warnings should be fixed, but...

Not only the developer is going to be using the plugins, people who are testing or even just using the latest will be, so you screw them up as well as the original developer :-(

And also these users should not need the testing tools installed to compile, we don't want to increase the dependencies.

And as for the original developer, they now have to build in the same way & use the same testing tools. That assumes they are using the same environment as the nightly builds, and I am not sure I even know exactly what that is.

Remember I'm only talking about development, I think the community has a right to ask for a higher standard for releases that the community is putting its name to. But during development, preventing compiling perfectly legal but frowned on code, is an imposition that I don't think the community currently wants to make (based on other comments in the thread).

...

...

...

In fact even errors should also not break the whole build, just the specific plugin.

agreed. But that's a somewhat different point I guess... maybe just tell people to use `make -k` ^^

Yep.

...

...

...

The problem here is that there is currently a plugin that can't be compiled with them, so enabling them would mean disabling the plugin that used to build.

Maybe the solution is to wait for Alexander to fix these problems, and then enable the "errors".

But the next patch commit on any plugin could fail one of the checks, so then the whole dev build fails again, thats no good, its got to still build with warnings, its a development build after all.

but again, what if I add

hello guys!

in the middle of the code? Or even more realistic (someone might even understand... :D)

if some test { foo_bar(); }

In this case, what would you do? Blame the programmer most probably. And you'd be right IMO :)

Of course it should fail the plugin, but as above, not the whole build.

And I hope we use "blame" in the non-emotive sense of "identify the cause of the problem" :-)

Maybe my English knowledge is not good enough, but I meant "beat down to death" :D [1]

Cheers, Colomban

[1] hopefully the reader will see the humor and understand I actually meant something like "gently ask the programmer to fix the problem"...

Yes, on the grounds that our worldwide army of automatic beating down robots has not been deployed yet, I guess we have to fall back to this :-)

Cheers Lex

On 03/11/11 18:33, Matthew Brush wrote:

On 03/11/11 18:21, Lex Trotman wrote:

...

I agree that warnings should be fixed, but...

Not only the developer is going to be using the plugins, people who are testing or even just using the latest will be, so you screw them up as well as the original developer :-(

Considering the plugin in question crashed (locked up) my entire Xorg session and I lost data, I can't say it would be a dis-service.

Replying to myself here...

I didn't mean to imply anything about that specific plugin, I actually think it's awesome. I mean with lesser experienced C programmers like me in the mix, having some additional checks against obvious programming errors might not be a bad idea :)

Cheers, Matthew Brush (codebrainz)

On Sat, 12 Mar 2011 02:53:47 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:

Unfortunately, believe me that non-fatal warnings are use to be ignored by unexperienced programmers, believing that if their code compile it is then OK. And I don't see why a warning upgraded to an error on every build would be worst than a syntactical problem (as I described above previously)? In a typical situation, the developer who writes the plugin should get the warning (well, the error), see his plugin don't build, care (hopefully :D), and then fix it directly even before committing and then before anybody else could face the problem. Don't you think?

Sorry I have not read all thread but here is my 2 cents. Warnings as errors are bad because different compiler version can generate different compile warnings. So It can be pain for developer to find and fix all warning for all compiler versions in the world. This issue is the same for for all other validation tools (valgrind, etc). Actually such maintains bother can be enough reason to abandon geany-plugins and move plugins to somewhere else.

On Sat, 12 Mar 2011 16:09:29 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:

...

This issue is the same for for all other validation tools (valgrind, etc). Actually such maintains bother can be enough reason to abandon geany-plugins and move plugins to somewhere else.

It would probably be sad, and it's not the goal, but we try to find a way to improve plugin quality. And to achieve this, we need to have some criteria. Of course the goal is not to enforce a ton of coding standards, validation process and stuff, and if a particular developer have a complain about something we will discuss it without problem.

But OTOH, if a plugin developer finds annoying to try to enforce a minimal quality on his plugin (the less crashers possible, not too many memory leaks, etc.), maybe geany-plguins don't want him. But I'm quite confident all developers want their code to be the better, so they would care :)

I second this :) Maybe the user also don't want to have his/her plugin either :)

Cheers, Frank

On 12 March 2011 20:21, Frank Lanitz frank@frank.uvena.de wrote:

On Sat, 12 Mar 2011 02:53:47 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:

...

...

Maaaybe, sort of see your point, but not really convinced that uprating warnings to errors is a good idea on the dev codebase, it stops people trying and testing things.

Unfortunately, believe me that non-fatal warnings are use to be ignored by unexperienced programmers, believing that if their code compile it is then OK. And I don't see why a warning upgraded to an error on every build would be worst than a syntactical problem (as I described above previously)? In a typical situation, the developer who writes the plugin should get the warning (well, the error), see his plugin don't build, care (hopefully :D), and then fix it directly even before committing and then before anybody else could face the problem. Don't you think?

Just thinking about adding a flag which is activating a paranoid check inside default build system and which lead into failing buildsin such a case. This would give plugin devs the chance to fix their code on one hand and unexperienced people which just want to test to have a build they can do their stuff with.

Yeah, so long as it doesn't make the whole system too complicated and is well documented, it has to be easily usable and maintainable :-)

Cheers Lex

Cheers, Frank -- http://frank.uvena.de/en/

---

Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel

On 12 March 2011 20:54, Frank Lanitz frank@frank.uvena.de wrote:

On Sat, 12 Mar 2011 20:49:10 +1100 Lex Trotman elextr@gmail.com wrote:

...

On 12 March 2011 20:21, Frank Lanitz frank@frank.uvena.de wrote:

...

On Sat, 12 Mar 2011 02:53:47 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:

...

...

Maaaybe, sort of see your point, but not really convinced that uprating warnings to errors is a good idea on the dev codebase, it stops people trying and testing things.

Unfortunately, believe me that non-fatal warnings are use to be ignored by unexperienced programmers, believing that if their code compile it is then OK. And I don't see why a warning upgraded to an error on every build would be worst than a syntactical problem (as I described above previously)? In a typical situation, the developer who writes the plugin should get the warning (well, the error), see his plugin don't build, care (hopefully :D), and then fix it directly even before committing and then before anybody else could face the problem. Don't you think?

Just thinking about adding a flag which is activating a paranoid check inside default build system and which lead into failing buildsin such a case. This would give plugin devs the chance to fix their code on one hand and unexperienced people which just want to test to have a build they can do their stuff with.

Yeah, so long as it doesn't make the whole system too complicated and is well documented, it has to be easily usable and maintainable :-)

Yes, didn't had something else in my mind. For autotools this could be a new target with cpp-flags as on waf it could be an option on command line. Something, and I bag you to don't take me to serious, similar to: make devbuild or ./waf build --developer

Does "waf build --developer" build developers? That would be great we could use some more!!! (well you said not to take it too seriously :-)

Cheers Lex

PS The question of course is does the nightly build use --developer, I'd say yes for all the reasons I've listed previously. But then by who and when is --release (or whatever its called) used?

Cheers, Frank

-- Frank Lanitz frank@frank.uvena.de

---

Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel

On Sat, 12 Mar 2011 15:55:52 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:

Le 12/03/2011 10:49, Lex Trotman a écrit :

...

On 12 March 2011 20:21, Frank Lanitz frank@frank.uvena.de wrote:

...

On Sat, 12 Mar 2011 02:53:47 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:

...

...

Maaaybe, sort of see your point, but not really convinced that uprating warnings to errors is a good idea on the dev codebase, it stops people trying and testing things.

Unfortunately, believe me that non-fatal warnings are use to be ignored by unexperienced programmers, believing that if their code compile it is then OK. And I don't see why a warning upgraded to an error on every build would be worst than a syntactical problem (as I described above previously)? In a typical situation, the developer who writes the plugin should get the warning (well, the error), see his plugin don't build, care (hopefully :D), and then fix it directly even before committing and then before anybody else could face the problem. Don't you think?

Just thinking about adding a flag which is activating a paranoid check inside default build system and which lead into failing buildsin such a case. This would give plugin devs the chance to fix their code on one hand and unexperienced people which just want to test to have a build they can do their stuff with.

Yeah, so long as it doesn't make the whole system too complicated and is well documented, it has to be easily usable and maintainable :-)

It would be easy to add a flag to enable/disable the warnings (actually I already had a flag to disable the warnings, thought they was enabled by default). However in Autotools side it's probably only really doable at configure-time, don't know for Waf.

So you'd prefer what:

1. enable warnings by default, and propose some to errors with a flag
2. no warnings by default, can be enabled by a flag (with or wtithout

error promotion?)

IMHO 1) is better so the developers gets the warnings, and it would still please everyone since these wouldn't be fatal -- only ugly :D.

If we finally agree with the flags I suggested minus the errors, I could set this up soon. Needs agreement (or not) then :)

I think we shall start with the compiling flags (I'm not this familiar with them so I cannot say which of them are really useful) with just warnings for the start. This should give a first impression to everybody where we are and some period we can do the next steps. As a new release for Geany-plugins is not scheduled yet it should also be fine to add them in a first step directly.

Cheers, Frank

On Sat, 12 Mar 2011 12:37:14 +1100 Lex Trotman elextr@gmail.com wrote:

Of course it should fail the plugin, but as above, not the whole build.

I agree.

And I hope we use "blame" in the non-emotive sense of "identify the cause of the problem" :-)

I agree².

Cheers, Frank

Le 11/03/2011 19:37, Colomban Wendling a écrit :

Le 08/03/2011 22:29, Colomban Wendling a écrit : I'd like to commit this to the Autotools build system:

1. run cppcheck on `make check`;
2. enable by default, if compiler understands them, the following

warnings (discussed in other mails of this thread):

• -Werror=implicit-function-declaration
• -Werror=pointer-arith
• -Wundef
• -Wshadow
• -Wwrite-strings

After thinking a little further, I updated my flags list first to remove error= stuff to fit the discussion result, and propose a few more, extracted from -Wall:

* -Warray-bounds: warns about some out-of-bound array subscripting * -Wformat: warns about wrong format/arguments in printf-like functions * -Wimplicit-int: warns if the return type of a function is not missing * -Wimplicit-function-declaration: warns if a function is not defined * -Wnonnull: warns when passing NULL as argument marked as non-nullable * -Wpointer-arith: warns about arithmetic usage of untyped pointers * -Wreturn-type: warns about missing return values in functions * -Wsequence-point: warns when the code might result to undefined result (e.g. "a[i++] = i") * -Wshadow: warns when shadowing symbols * -Wstrict-aliasing: warns when breaking strict aliasing rules [1] * -Wstrict-overflow=1: warns when breaking strict overflow rules [1] * -Wtrigraphs: warns about trigraphs that may be interpreted * -Wundef: warns when testing undefined constants with the preprocessor * -Wuninitialized: warns when detecting use of undefined varaibles * -Wunused-value: warns when computing an unused value * -Wunused-variable: warns about unused variables * -Wwrite-strings: makes string literals constant, used to find case where string literals are used as a modifiable value, which should be avoided.

All of these but -Wpointer-arith, -Wshadow, -Wundef and -Wwrite-strings are part of GCC's -Wall. Maybe we might directly use -Wall, but it warns about some things that are not really needed, such as unused functions.

Actually, apart -Wwrite-strings, these flags don't show so much warnings :)

So (once again), do these flags seems reasonable to you?

Cheers, Colomban

[1] see GCC's manual for details ;)

Le 12/03/2011 23:49, Frank Lanitz a écrit :

On Sat, 12 Mar 2011 23:34:56 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:

...

Maybe we might directly use -Wall, but it warns about some things that are not really needed, such as unused functions.

Well, I'd like to get informed about unused functions.

Oh, so we might want to simply use -Wall then -- though it also adds -Wswitch that warns for unhandled enumeration values if the is no "default:" (it is useful in some case but might produce tons of warnings that aren't really important).

Anybody against it?

Cheers, Colomban

Le 13/03/2011 02:38, Lex Trotman a écrit :

On 13 March 2011 09:34, Colomban Wendling lists.ban@herbesfolles.org wrote:

...

[...]

• -Warray-bounds: warns about some out-of-bound array subscripting
• -Wformat: warns about wrong format/arguments in printf-like functions
• -Wimplicit-int: warns if the return type of a function is not missing

Actually warns when the function type IS missing, but know what you mean :-)

Woops! I must have reworded my sentence incorrectly :D

...

[...]

All of these but -Wpointer-arith, -Wshadow, -Wundef and -Wwrite-strings are part of GCC's -Wall. Maybe we might directly use -Wall, but it warns about some things that are not really needed, such as unused functions.

Actually, apart -Wwrite-strings, these flags don't show so much warnings :)

So (once again), do these flags seems reasonable to you?

Well if they are warnings only, the more the merrier, I'd use -Wall and -Wextras but noting that there is a possibility that some plugin might need a customised list if there is a good reason why it is correct and can't reasonably avoid some warning. Up to the plugin dev to tell us why, but hey we're reasonable aren't we? :-)

The problem of -Wextra is that it adds -Wunused-parameter, which is a bit annoying when writing callbacks (you get tons of them, and you don't have to fix them). But it adds some cool stuff too (for example -Wmissing-parameter-type and -Wold-style-declaration). Maybe we would like to use -Wall -Wextra -Wno-unused-parameter, or only pick the warning we like from -Wextra.

I don't think we should add too much warnings with false-positive (like -Wunused-parameter) by default, because they are likely to: * make the unexperienced programmer try to fix them, though actually they shouldn't be [1]; * "hide" some more interesting warnings between plenty of false ones; * annoy some developers without good reason.

Note most developers compile Geany itself with -Wall at least (iaw the Hacking file) and Nick has some extras he likes.

Of course -- I personally use the following for all my builds: :D -Wall -W -Wunused -Wunreachable-code -Wformat=2 -Wundef -Wshadow -Wpointer-arith -Wbad-function-cast -Wwrite-strings -Wconversion -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -Wmissing-noreturn -Wmissing-format-attribute -Wredundant-decls -Wnested-externs -pedantic -Werror-implicit-function-declaration -fshow-column

But these are maybe a little too much, they usually generate a bunch of warnings that should not be "fixed" since they are false-positive actually.

Also note that some warnings need at least -O2 so the compiler does the dataflow tracing needed to detect the warning.

Right, but -O2 also have some drawbacks when debugging, so I'm not sure we want to add it by default.

PS we are only talking about gcc flags here but I think we only supprt gcc even on Windows, right Enrico?

Actually I check whether the compiler understand each flag, so it would be easy to support any compiler. I only talk about GCC warnings because I only know GCC's flags, but if somebody knows some other, we might add them.

Cheers, Colomban

[1] for example, hiding an unused parameter warning by faking a usage (like "(void)param") generally only bloats the code without any benefit

On Sun, 13 Mar 2011 03:57:59 +0100, Colomban wrote:

Hey,

I didn't read all of the thread, so I might be missing something. But still.

...

PS we are only talking about gcc flags here but I think we only supprt gcc even on Windows, right Enrico?

Nope. We try to support ANSI C and so support any compatible compiler out there. Though, some years ago I tried compiling with MSVCC on Windows and Geany didn't compile out-of-the-box. I don't remember the exact problems but I also didn't care enough. MSVCC is probably not worth to be supported :D. But there are still other compilers like Intel's icc or suncc (is it called suncc? well, the compiler on Solaris).

Actually I check whether the compiler understand each flag, so it would be easy to support any compiler. I only talk about GCC warnings because I only know GCC's flags, but if somebody knows some other, we might add them.

Do you want to integrate these flags into the build system? I don't think this is a good idea. Such flags should be set outside of the build system by the developer/user, not automatically. This is why they are mentioned in HACKING.

We could maybe add a little wrapper script into the scripts/ directory which sets a bunch of compiler options we like, for convenience.

Below is a list of flags I currently use for Geany though I didn't touch this list for months actually.

export CFLAGS="-Wall -O2 -g -pipe -march=athlon64 \ -D_FORTIFY_SOURCE=2 \ -DGEANY_DEBUG \ -DGEANY_DISABLE_DEPRECATED \ -fno-common \ -funit-at-a-time \ -Waggregate-return \ -Wdeclaration-after-statement \ -Wdisabled-optimization \ -Wfloat-equal \ -Wformat=2 \ -Wformat-nonliteral \ -Wformat-security \ -Winit-self \ -Winline \ -Wmissing-field-initializers \ -Wmissing-format-attribute \ -Wmissing-include-dirs \ -Wmissing-noreturn \ -Wmissing-prototypes \ -Wnested-externs \ -Wpointer-arith \ -Wredundant-decls \ -Wshadow \ -Wsign-compare \ "

Regards, Enrico

Hi,

On Sun, 13 Mar 2011 09:57:36 +0100 Enrico Tröger enrico.troeger@uvena.de wrote:

Do you want to integrate these flags into the build system? I don't think this is a good idea. Such flags should be set outside of the build system by the developer/user, not automatically. This is why they are mentioned in HACKING.

We could maybe add a little wrapper script into the scripts/ directory which sets a bunch of compiler options we like, for convenience.

Thanks for bringing the point back up to my mind. I totally missed that there are other thinks beside gcc outside.

Based on this having a separate script is a good idea.

Cheers, Frank

On 11.03.2011 19:37, Colomban Wendling wrote:

Do you have objections, comment or whatever?

I largely agree with all of what Lex said.

* I routinely add temporary debug code without the proper headers (because I know it works) so making this break the build would be inconvinient for me (and perhaps other experienced programmers) * I agree with more warnings, though, generally * I think they should be in the build system. Having them on the developers site has only created problems before, such as new devs simply missing them

On 9 March 2011 09:11, Matthew Brush matthewbrush@gmail.com wrote:

On 03/08/11 10:58, Enrico Tröger wrote:

...

As I'm currently reworking the system to create the nightly builds, we could integrate such checks into the nightly builds, maybe run cppcheck on the sources after the builds and present the results somewhere on nightly.geany.org.

A couple ideas:

1. Parse cppcheck XML output with Python[1] (or whatever) and remove any

failing plugins from the main Makefile.am listed in the 'plugins' variable so they don't build.

Disagree with removing plugins from nightly builds, testing will find many more errors than these checks will, and removing the plugin makes it unavailable for testing. I would support having failing plugins as a second tier release, but never remove access completely.

Cheers Lex

2. Use the HTML report[1] script (uses awesome Pygments) for displaying

results as you mentioned.

They have already written most of the code for both ideas.

[1] https://github.com/danmar/cppcheck/blob/master/htmlreport/cppcheck-htmlrepor...

Cheers, Matthew Brush (codebrainz) _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel

Hi,

Am 23.02.2011 01:10, schrieb Matthew Brush:

Another thing could be to make mandatory that documentation is existent and current, up to some standard. I mean for README, manual, and also doc-comments in code (ex. each function/global must have a comment or something). Some other items where documentation could be enforced; new keybindings, preferences, menu/toolbar items, tabs.

I agree in a general view. According to code comments I'm not sure. But can you give me some more details what did you think about when you referred to the keybindings, preferences etc.?

I know its not possible for everybody and useful in every case, but I did try to make on geanyLaTeX docu [1] what I'd like to have for every plugin.

Cheers, Frank

[1] http://frank.uvena.de/files/geany/manuals/geanylatex/geanylatex.html

On Wed, 09 Mar 2011 07:27:06 -0800 Matthew Brush matthewbrush@gmail.com wrote:

On 03/09/11 03:42, Frank Lanitz wrote:

...

Hi,

Am 23.02.2011 01:10, schrieb Matthew Brush:

...

Another thing could be to make mandatory that documentation is existent and current, up to some standard. I mean for README, manual, and also doc-comments in code (ex. each function/global must have a comment or something). Some other items where documentation could be enforced; new keybindings, preferences, menu/toolbar items, tabs.

I agree in a general view. According to code comments I'm not sure. But can you give me some more details what did you think about when you referred to the keybindings, preferences etc.?

Sure! The perfect example, although I haven't tried it in a little bit, is the Debugger plugin. You load the plugin and see the tab in the Message Window, and it looks really cool and useful. Obviously I wanted to try it out right away, so I start to hover over the GUI elements hoping for some tooltips to guide me to what to do, but nope. There was nothing in the README (at least when I tried). So then I tried to just figure it out. Finally after about 10 minutes I figure out that "Target" means the binary executable you want to debug, which seems kinda obvious now, but at the time the wording alone of "target" didn't seem to cut it.

So after that now I want to run it. I spent another 10-15 minutes looking through the Toolbar, Build menu, Tools menu, etc looking for where I can invoke the Debugger. There was nothing in the README. Finally I looked under the Keybindings preferences and saw there was keybindings to run it, and there was no default keybindings selected.

Point of that story is that *all* I needed was a few hints about what the plugin added to the GUI and what I needed to do to invoke it. Things like the new keybindings, their defaults, etc. Personally, after using some other IDEs, I would have expect a toolbar menu button with "step into", "step out of", etc. So I guess what I mean is, if you add a keybinding, or a preference, or a menu item or toolbar item, it should be documented.

I don't mean to pick on the Debugger plugin, I think it's *really* cool and I can appreciate that it's still under development, but this is specifically what I was thinking of when I wrote that in my previous message.

Ah I see. So basicly what you refer to is having a suitable documentation at all with most important things as well as ensure there is a basic 'ingame' docu. Did I understand it correct?

Cheers, Frank

On Fri, 11 Mar 2011 15:36:50 -0800 Matthew Brush matthewbrush@gmail.com wrote:

On 03/11/11 14:43, Frank Lanitz wrote:

...

On Wed, 09 Mar 2011 07:27:06 -0800 Matthew Brushmatthewbrush@gmail.com wrote:

...

On 03/09/11 03:42, Frank Lanitz wrote:

...

Hi,

Am 23.02.2011 01:10, schrieb Matthew Brush:

...

Another thing could be to make mandatory that documentation is existent and current, up to some standard. I mean for README, manual, and also doc-comments in code (ex. each function/global must have a comment or something). Some other items where documentation could be enforced; new keybindings, preferences, menu/toolbar items, tabs.

I agree in a general view. According to code comments I'm not sure. But can you give me some more details what did you think about when you referred to the keybindings, preferences etc.?

Sure! The perfect example, although I haven't tried it in a little bit, is the Debugger plugin. You load the plugin and see the tab in the Message Window, and it looks really cool and useful. Obviously I wanted to try it out right away, so I start to hover over the GUI elements hoping for some tooltips to guide me to what to do, but nope. There was nothing in the README (at least when I tried). So then I tried to just figure it out. Finally after about 10 minutes I figure out that "Target" means the binary executable you want to debug, which seems kinda obvious now, but at the time the wording alone of "target" didn't seem to cut it.

So after that now I want to run it. I spent another 10-15 minutes looking through the Toolbar, Build menu, Tools menu, etc looking for where I can invoke the Debugger. There was nothing in the README. Finally I looked under the Keybindings preferences and saw there was keybindings to run it, and there was no default keybindings selected.

Point of that story is that *all* I needed was a few hints about what the plugin added to the GUI and what I needed to do to invoke it. Things like the new keybindings, their defaults, etc. Personally, after using some other IDEs, I would have expect a toolbar menu button with "step into", "step out of", etc. So I guess what I mean is, if you add a keybinding, or a preference, or a menu item or toolbar item, it should be documented.

I don't mean to pick on the Debugger plugin, I think it's *really* cool and I can appreciate that it's still under development, but this is specifically what I was thinking of when I wrote that in my previous message.

Ah I see. So basicly what you refer to is having a suitable documentation at all with most important things as well as ensure there is a basic 'ingame' docu. Did I understand it correct?

I'm not sure what "'ingame' docu" means but what I mean is there should be a few minimal requirements, such as:

• Tooltips on widgets added to the UI (where it makes sense)
• A plugin_help() implementation, even if it just links to the

README on plugins.geany.org

• A README file with text content including
  • The purpose of the plugin, what it accomplishes, etc.
  • A list of keybindings that will be available when the plugin

is loaded - A usage section explaining at least the basic way to use the plugin - A description of the new menu items, tool items, and so on introduced to the UI.

This is just some ideas...

Yepp, that's what I understood under 'ingame' ;)

On Tue, 22 Feb 2011 21:40:09 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:

Le 22/02/2011 20:04, Thomas Martitz a écrit :

...

Am 22.02.2011 19:36, schrieb Colomban Wendling:

...

...

So my 1st suggestion is to remove all plugins which do have known issues and don't compile with some -W-flags (needs to be defined) from common build until these are fixed. Also remove plugins which don't bring propper documentation as well are unmaintained for some time.

Well... I'm puzzled. Disabling unperfect plugins is a good idea in theory for the stability of Geany, but... how would be plugins tested then?

However, I agree with making official builds with strong error checking enabled (at least -Werror-implicit-function-declaration comes directly to my mind). This is static checking that will show most obvious problems, and hopefully it'll encourage their maintainer to fix them. However, finding the right flags will not be easy... for example not allowing any implicit cast may have a false-positive effect where unexperimented developers might hide a part of those with a cast where a proper fix would be better.

I agree. While we may know they have known problems, how would we know in the future if they're disabled and receive no testing?

I think all plugin developers should get more easy about others touching there code. I could probably fix a number of problems in other people's plugins, but I'm afraid of the surrounded discussion (post or pre commit).

I don't think that touching other one's code without her agreement is a good idea. There are several reasons for this that comes to mind:

• some people will find this offensive (e.g. "you can't even code

right and you won't even understand if I tell you");

• some people will find it cool, but won't necessarily try to

understand any further (heh, it's already fixed), so would likely reproduce mistakes later;

• and unfortunately, some people might change things with not enough

care or understanding and break other things -- this is easier than we might think...

• and some people would not feel confident of somebody else commit --

and this would probably be my case if it's somebody I hardly know.

Well, I'm afraid this might all can happen. But call me green I don't think this will happen too often and cannot be solved in case of it does happen and ppl like to talk to each other. If somebody who is submitting a patch keeps in mind that he is not the main author. On the other hand ... I don't know. All these point do make me unsure.

And AFAICT, people who would need help are generally likely to accept it when proposed (probably more than if it was imposed).

Everything based on how you offer help I guess.

...

What I also wonder; is there a way to prevent plugins from crashing geany entirely? I'd rather have a notice "Oops, Plugin Foo crashed".

I doubt. This would need to run plugins in a separate process, and then doing some IPC to communicate. Probably doable (well, AFAIK recent web browsers tend to do so with their plugins -- chromium, FF4) but probably really not easy.

And surely nothing we like to have in Geany I guess.

Cheers, Frank

Hit enter too early :)

Am 22.02.2011 19:36, schrieb Colomban Wendling:

Finally, I don't point my finger to anybody neither, but I know some of the developers aren't experienced C developers. They then probably cannot really review their own code for C problems.

I also observed this, and I think that's a major part of the problem. Hopefully we can code plugins in friendlier languages, such as Vala or python soon (work on both is ongoing).

Best regards.

Am 22.02.2011 23:00, schrieb Frank Lanitz:

...

I also observed this, and I think that's a major part of the problem. Hopefully we can code plugins in friendlier languages, such as Vala or python soon (work on both is ongoing).

Even in Vala or Python you can write bad code... I'm very experienced in doing it on Python...

Cheers, Frank

It's sure possible. But it's not as easy. Can you free("do not free string literals") in python? :)

I don't think we need to argue whether or not C is a hard language for beginners.

Best regards.

Hi,

On Tue, 22 Feb 2011 19:36:51 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:

Hi,

Le 22/02/2011 17:28, Frank Lanitz a écrit :

...

Hi folks,

And again it happen to me that Geany did crash due some issue with a plugin which might not have been tested very well before checking in /committing. However, I don't want to point with my finger to any developer so I'm asking how we could improve overall quality of plugins inside release as well as on trunk.

Failures I was recognizing in past did start with simple case/data type warnings on compile time up to segfaults caused by not correct initialized pointers as well a number of memory leaks. I know its not possible to write 100% clean code as well as I'm aware of my code isn't very well too. But I really like to change something here.

So my 1st suggestion is to remove all plugins which do have known issues and don't compile with some -W-flags (needs to be defined) from common build until these are fixed. Also remove plugins which don't bring propper documentation as well are unmaintained for some time.

Well... I'm puzzled. Disabling unperfect plugins is a good idea in theory for the stability of Geany, but... how would be plugins tested then?

This is something I like to get clarified with my question ;)

However, I agree with making official builds with strong error checking enabled (at least -Werror-implicit-function-declaration comes directly to my mind). This is static checking that will show most obvious problems, and hopefully it'll encourage their maintainer to fix them. However, finding the right flags will not be easy... for example not allowing any implicit cast may have a false-positive effect where unexperimented developers might hide a part of those with a cast where a proper fix would be better.

I agree.

And even I (yes, I think I'm pretty experimented and knowledgeable about C) sometimes don't correct a hard-to-fix integer cast warning because I know this particular implicit cast isn't really important but I also know it should better be fixed than simply hiding it with an explicit cast. So I leave the warning, kinda as a "rememberer".

So... you know your task ;)

...

As a first shoot this could be from my point of view

• Updatechecker (crash on Windows build)

It then might be disabled only for Windows if it's safe on Unices?

Yes. Valid point.

...

• GeanyLUA (lot of warnings on compiling time)
• Pretty Printer (lot of warnings on compiling time)

Why not. Hopefully their author will fix them :)

Nick did take it over as the original developer didn't maintain it anymore. There have been only the most important things done IIRC due of this point.

...

• GeanyGDB (nearly unmaintained)

For this one, it also has a candidate for replacement, hasn't it?

Yepp. But this would also include a 100% review uf the new plugin and check whether it really does its job. At least for me I cannot decide.

...

• GeanyVC (de facto unmaintained)

...but AFAICT it's stable. Unmaintained software isn't good, but I doubt dropping such a feature would please all users.

There are some issue with the plugin. Of course a huge number of poeple would miss it.

...

• WebHelper (Crash mentioned on SF)

This is a good (I think :D) example of what I talked about above: user testing. I use this plugin, and at least load it often (heh, I wrote it...) and never found a crash I didn't fixed. So without testing, I would probably never have been informed of such a crash, so I'd never had a chance to fix it.

Yes, thats true. Without good testing you cannot find all bugs.

...

Unfortunately this is not a complete list. So maybe we could just remove every plugin and just start to create a white list.

Again, a false-positive of a white-list is that "blacked" plugins won't get real testing.

This is not necessary true. It would be inside mainstream and would be missing this testing, thats correct. But maybe there is some other process in place?

...

Ideally (from my point of view) we could set up some kind of review process but I'm afraid it will not possible due lag of resources - somebody needs to read and understand the code.

Well... I might do basic review on demand. Not necessarily understanding all the code, but reading it as a C developer and reporting what I find wrong. Such a basic review probably needs about 1 or 2 hours, but I personally feel this kind of code review more as "rest time" than "hard work" ;)

I always like writing code more than try to understand others. Just a thought ... Maybe you can do some review on some of the other plugins if you have some need for a rest? ;) Would be really cool I guess.

...

Also we might could work with tests for typical things - somebody only have to code them. I just don't see any big alternatives at the moment.

Like what? I doubt a specific tool would have most benefit over more generic (and already written :p) tools -- I think of a static code checker, maybe cppcheck or clang --check; and the Runtime Error Checking King, I named Valgrind.

I did have a look onto Valgrind... well.... I started to have a look but I wasn't able to figure out how its working in a useful way. Some more playing around is needed there I guess.

Finally, I don't point my finger to anybody neither, but I know some of the developers aren't experienced C developers. They then probably cannot really review their own code for C problems. And anyway, an external review is IMHO always a good thing, even for the most experienced of us: at least it's an opportunity to justify (to ourselves) how the code is written.

I agree.

...

What do you think about?

To summarize: a white list is a kinda bad idea IMHO since it'll avoid user testing. However, I think it's a good idea to enforce code quality checking.

I agree on lot of user testing might will be missing but due some of the bugs experienced with geany-plugins 0.20 I assume its not too much missing :( Beside of this I totally second your opinion.

Cheers, Frank