On 21/08/2024 20:23, Kyle Lippincott wrote:
On Wed, Aug 21, 2024 at 11:46 AM Calvin Wan <calvinwan@xxxxxxxxxx> wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
I'm suprised the compiler does not complain that 'struct config_set' is
not declared in this header - I was expecting to see
struct config_set;
I'm surprised as well actually. Removing the forward declaration of
"struct config_set" in repository.h doesn't result in any complaints
from the compiler either. Will add it in the reroll, but am curious if
anyone has any ideas why the compiler isn't complaining.
C doesn't require structs be forward declared separately. You can
change the name to be anything you want, and as long as the forward
declaration of the function and the function definition agree, you're
fine.
Oh, TIL. The declaration
struct foo *f(struct bar*);
declares "struct foo" and "struct bar" as well as function "f". The
declaration of "struct bar" is scoped to the parameter list and so is
not visible to the rest of the file. This is why we forward declare
structs that are used in parameter lists and why the compiler complains
if we don't. The declaration of "struct foo" does have file scope though
which explains why the compiler does not complain about
public_symbol_export.h because "struct config_set" is declared as a
return value before it is used in any parameter lists.
Thanks
Phillip
If they don't agree, well, let's hope you don't encounter that
(it's the same problem as if you have a forward declaration that's
*not* visible from the definition that disagrees with the definition:
`void some_func();` vs. `int some_func(int arg) { ... }` -- if the
forward declaration wasn't made in the same translation unit that
defines `some_func`, nothing detects this misuse in C).
For this reason, you should only ever use forward declarations that
are provided by "the code" that's being forward declared. i.e. if
you're trying to use a function from foo.c, please forward declare it
in foo.h, and only there. This way, assuming foo.c includes foo.h,
you'll detect mismatches.
[apologies if people got multiple copies of this, I sent with HTML
mode enabled the first time]
before the function declarations. As I said in my comments on the last
patch I think we'd be better to namespace our types as well as our
functions in this library layer so this can be resued by other language
bindings.
Are you suggesting something like "#define libgit_config_set
config_set"? I wouldn't be comfortable renaming config_set in git.git
just yet until config/config_set can be a standalone library by itself.
> [...]
+ pub fn get_str(&mut self, key: &str) -> Option<CString> {
If we're adding an ergonomic api then having return CString isn't ideal.
I think the equivalent function in libgit2-rs has variants that return a
String which is convinent if the caller is expecting utf8 values or
Vec<u8> for non-utf8 values.
Having both get_cstr() and get_str() makes sense to me.
