On 05/09/2013 09:41, Matthew Brush wrote:
On 13-09-04 09:20 AM, Nick Treleaven wrote:
My C89 & C++ version:
guint i; foreach_document(i) { GeanyDocument *doc = documents[i]; // do stuff with every valid doc }
While this code is short, it's actually sort of nuts too (and also not very C++-ish).
- You should include the definition of the custom macro used and the
definitions of the two other non-obvious macros used inside the first macro for a fair comparison, since none of them are standard or even well-known like the pasted examples.
Most files already include document.h.
- The code does not loop over the windows like the pasted code, so
you'd need another outer loop which would likely have a 2nd style of iteration wrapped around the existing loop.
We don't support accessing multiple windows IIUC.
BTW I was just providing code for that specific case, not necessarily for all iteration.
- It's somewhat unclear what type should be passed in looking at the
macro without the doc comments (I always forget and have to RTFM each time I see it since we often use gint where we mean guint).
- The argument is not only assigned to but also evaluated multiple
times (what if you pass `--i` or something weird as argument). I guess assigning might be good here since the compiler would catch it rather than flying off the end of the array when it hits `(guint)-1` (UINT_MAX) on underflow.
It's fairly tricky to debug misuses of the macro (better with Clang).
The implicit check in the loop (failure first) indexes into a macro
wrapper around a whole other type using a hard cast from the result of another function-like macro cast thing off in another file, and is dereferenced without any NULL check (although probably safe in this case, unless a plugin or something re-assigns the global variable or redefines any macro aliases of it).
- It forces you to define an indexing variable outside of the loop into
the wrong scope (C89-style, even if used in > C99 plugin code).
- Even though slightly dirtier, it would be more useful to iterate over
document pointers than document indexes (what are those? tab pages? order of opening? arbitrary array indices?), for example (untested, C99 or GNU89 probably):
#define foreach_doc(doc) \ for (unsigned z_=0; z_ < documents_array->len; z_++) \ if ( !((doc) = documents_array->pdata[z_]) || \ !(doc)->is_valid ) { continue; } else ... GeanyDocument *doc; foreach_doc(doc) { // every valid doc }
Yes, that is much better if we have C99/C++. That also solves most of the points above.
- I personally find it annoying to use API's that have all their own
versions of things that are really common and not that hard anyway; it's difficult to learn them all, and also all the stuff mentioned above when they are macros. Consider this slightly longer example that uses no macro madness and common C looping idiom:
guint i; for (i = 0; i < documents_array->len; i++) { GeanyDocument *doc = documents_array->pdata[i]; // do stuff with every *should be* (but isn't) valid doc }Assuming the API didn't have the weirdness about documents being invalid but still sticking around for whatever reason, it's the same number of lines as the macro version and no magic.
Yes. Although sometimes bugs do slip through, even in incrementing for loops. Suppose someone used <= instead of <, or forgot to initialize i.
- The macro should be named `foreach_document_index()`.
OK.
I don't think we should rewrite Geany's API in C++, or maintain a C++ wrapper for the C one, except for any cases which are particularly bug-prone.
An idiomatic C++ binding would be quite useful for a plugin written in normal C++. In my current C++ plugin I'm working on, I'm actually wrapping up a few parts of Geany's API it uses to avoid scattering the weird C code containing lots of raw pointers, miscellaneous allocators, different string/collection types, and so on, throughout the normal C++ code.
It would be useful, but also a distraction for the core project.