Re: [PATCH v2 5/5] cgit: add higher-level cgit crate

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 22, 2024, 13:24 UTC
Message-ID: <27a37848-cdf8-46bb-9764-d8f952504c12@gmail.com>
In-Reply-To: <CAO_smViznLAxxGH+rfouUquq8tNJn16-CSoLbbGosy3fuYzTgQ@mail.gmail.com>

On 21/08/2024 20:23, Kyle Lippincott wrote:

Show 18 quoted lines

> On Wed, Aug 21, 2024 at 11:46 AM Calvin Wan <calvinwan@google.com> wrote:
>>
>> Phillip Wood <phillip.wood123@gmail.com> 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

Show 37 quoted lines

> 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.
>