For what it's worth, "internal" appears to mean "is in the ABI, but not in the header" - so we're relying on C Folks users' use of -Werror=implicit to avoid things that are not considered to be public API working anyway. I was hoping it did tricks with G_GNUC_INTERNAL, but apparently not.

This appears to be sufficient to fix the build with -Werror=implicit:

-  public bool is_user_set_default { get; internal set; default = false; }
+  public bool is_user_set_default { get; protected set; default = false; }

(In reply to comment #1)
> For what it's worth, "internal" appears to mean "is in the ABI, but not in the
> header" - so we're relying on C Folks users' use of -Werror=implicit to avoid
> things that are not considered to be public API working anyway. I was hoping it
> did tricks with G_GNUC_INTERNAL, but apparently not.
> 
> This appears to be sufficient to fix the build with -Werror=implicit:
> 
> -  public bool is_user_set_default { get; internal set; default = false; }
> +  public bool is_user_set_default { get; protected set; default = false; }

IIRC, is_user_set_default’s setter was intentionally created as ‘internal’ so that it wasn’t exposed in the header, since we don’t want client programs accidentally calling it. At the time I assumed that ‘internal’ visibility did some tricks with G_GNUC_INTERNAL, and didn’t look into it further.

The problem with changing it to ‘protected’ visibility is that it’s equivalent to ’public’ visibility as far as C code is concerned. We were trying to avoid that, since it seems unnecessary.

I think the proper fix would be to improve Vala to use G_GNUC_INTERNAL properly. I’d prefer to see that than make is_user_set_default a protected setter.

In the mean time, please feel free to commit a change to folks which uses -Werror=implicit everywhere except in the EDS backend. I guess it would be best to add -Werror=implicit to (a new) ERROR_CFLAGS, and add -Wno-error=implicit (with a FIXME) to the EDS backend’s Makefile.am.

(In reply to comment #2)
> I think the proper fix would be to improve Vala to use G_GNUC_INTERNAL
> properly. I’d prefer to see that than make is_user_set_default a protected
> setter.

Sorry, that won't work. G_GNUC_INTERNAL means "local to this shared library", so libfolks-eds can't see G_GNUC_INTERNAL symbols from libfolks.

To have a symbol that is visible to libfolks-eds but not to "the general public", it would have to be ABI but not API: in other words, not G_GNUC_INTERNAL, but not in an installed header either. (folks_small_set_new() and folks_internal_equal_sets() are examples of such symbols.)

I'm not sure that is_user_set_default being internal actually gains us much in terms of symbol visibility, because as far as I can see, a library user can equally easily go via GObject properties (which have no visibility or access control):

    g_object_set (store,
        "is-user-set-default", TRUE,
        NULL);

(To prevent this it would have to be read-only or construct-only at the GObject level, but I don't think Vala gives us that much control - "internal set" seems to be translated into G_PARAM_WRITABLE.)

(In reply to comment #2)
> I think the proper fix would be to improve Vala to use G_GNUC_INTERNAL
> properly.

That's Bug #669531, although if it's implemented, we'd have to stop using "internal" among different binaries from the same source tree. See also Bug #694809.

(In reply to comment #3)
> I'm not sure that is_user_set_default being internal actually gains us much in
> terms of symbol visibility, because as far as I can see, a library user can
> equally easily go via GObject properties (which have no visibility or access
> control):
> 
>     g_object_set (store,
>         "is-user-set-default", TRUE,
>         NULL);

That's Bug #653909 (but you already know that, since you reported it).

Created attachment 241236 [details] [review]
Re-brand ERROR_CFLAGS to C_ERROR_CFLAGS

These are flags that we only use when compiling hand-written C code,
mostly originating from telepathy-glib. We can't be that strict with
the C code generated by Vala, because it's full of questionable things
(which the Vala compiler verifies are OK, but the C compiler still warns
about).

Created attachment 241237 [details] [review]
Include ERROR_CFLAGS everywhere we compile C code

AM_CFLAGS was missing from a couple of foo_CFLAGS variables (it isn't
"inherited" automatically), so add that too.

Created attachment 241238 [details] [review]
Declare internal function  folks_persona_store_set_is_user_set_default()

As explained in the comment, I can't find any better way to do this.
Implicit declarations are Bad, so we should be making them into errors
by default.

---

See the (long) comment in the code. I am not very happy with this but I can't see a way to do better.

See also Bug #694809.

Created attachment 241239 [details] [review]
Ensure that the C bits of TestCase are pre-declared in a  header

This lets us verify that their Vala and C ABIs are the same: if they
are not, compilation will fail.

---

Caught by -Werror=missing-declaration, which checks that every function has a prior declaration (typically in a header file; but you can just declare it repetitively in its own .c file, like valac does, if you're desperate).

Created attachment 241240 [details] [review]
Add warnings (normally fatal) on use of archaic C  features

We don't want these even in C code generated by Vala, and certainly not
in hand-written C code.

Review of attachment 241236 [details] [review]:

++

Review of attachment 241237 [details] [review]:

++

Review of attachment 241238 [details] [review]:

I guess this is a reasonable solution, if a little onerous to maintain.

::: folks/redeclare-internal-api.h
@@ +30,3 @@
+ * header or the "public" one, never both. If we use the "internal"
+ * VAPI then libfolks-eds' "public" header ends up trying to include
+ * the "internal" header of libfolks, which is unacceptable.

I suppose there’s scope for Vala to grow a --internal-header-without-typedefs option which doesn’t contain redefinitions of public typedefs.

@@ +42,3 @@
+
+void folks_persona_store_set_is_user_set_default (FolksPersonaStore* self,
+    gboolean value);

Probably a good idea to add a comment in persona-store.vala pointing here.

Review of attachment 241239 [details] [review]:

++

Review of attachment 241240 [details] [review]:

++ \o/

(In reply to comment #3)
> To have a symbol that is visible to libfolks-eds but not to "the general
> public", it would have to be ABI but not API: in other words, not
> G_GNUC_INTERNAL, but not in an installed header either. (folks_small_set_new()
> and folks_internal_equal_sets() are examples of such symbols.)

Side note on this: I do intend it to be possible for SmallSet and its unfinished relatives SmallMap and SmallMultiMap to be API one day (although it seems to be necessary to have them in their own .vapi for that to work) so that out-of-tree backends can use them, which is why small-set-internal.h exists.

However, it doesn't seem urgent to have them be API yet (and having them as non-API gives us the flexibility to change constructor parameters, etc. if desired). In particular, if they do become API I'd want to disable separate headers when FOLKS_COMPILATION is not defined, and add a meta-header <folks/folks-generics.h> or something, analogous to <telepathy-glib/telepathy-glib.h>.

Everything applied, with a comment added as you suggested:

+  /* The setter folks_persona_store_set_is_user_set_default() is redeclared
+   * in folks/redeclare-internal-api.h so that libfolks-eds can use it.
+   * If you alter this property, check the generated C and update that
+   * header if necessary. https://bugzilla.gnome.org/show_bug.cgi?id=697354 */
   /**
    * Whether this {@link PersonaStore} has been marked as the default
    * store (in its backend) by the user. I.e.: a PersonaStore for the e-d-s