Prefix for associated constants

[kvark]

We are looking at the bitflags output of this struct:

bitflags! {
    #[repr(transparent)]
    pub struct TextureUsageFlags: u32 {
        const TRANSFER_SRC = 1;
        const TRANSFER_DST = 2;
        const SAMPLED = 4;
        const STORAGE = 8;
        const OUTPUT_ATTACHMENT = 16;
        const PRESENT = 32;
        const NONE = 0;
        const WRITE_ALL = 2 + 8 + 16;
    }
}

The result looks like this:

#define WGPUOUTPUT_ATTACHMENT (TextureUsageFlags){ .bits = 16 }
#define WGPUPRESENT (TextureUsageFlags){ .bits = 32 }

There is "WGPU" prefix attached (which we specify in the options, but it's only means for types), and instead we'd want the type name to be the prefix, i.e.

WGPUTextureUsageFlags_OUTPUT_ATTACHMENT

The current output is not very usable - it's not desired to have the type prefix affecting associated constants, and the way they are generated now there appears to be conflict between types (i.e. we have similar constants for BufferUsageFlags bitflags, and they overlap).

cc @grovesNL @eqrion @IGI-111

[IGI-111]

Ah yes, that is one of the caveats, no explicit namespacing.

It should be relatively simple to add some (like a type prefix as you mention), but we should probably discuss the edge cases and document the specifics, because it's necessarily cbindgen specific behavior.

[kvark]

Do you have any concerns (edge cases) in mind?

…

On Oct 22, 2018, at 12:01, IGI-111 ***@***.***> wrote: Ah yes, that is one of the caveats, no explicit namespacing. It should be relatively simple to add some (like a type prefix as you mention), but we should probably discuss the edge cases and document the specifics, because it's necessarily cbindgen specific behavior. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[IGI-111]

Mostly collisions. Between namespaces and types, between classic constants and associated variable constants and so on.
#define collisions are especially hairy to debug so I'm inclined to be careful.

I'm certainly not opposed to just doing the least surprising behavior by default, i.e.: just adding a type prefix. But we can probably avoid some pains with an optional prefix for typenames for instance. Something like

type_prefix = "__" # "" by default

That'd make WGPU__TextureUsageFlags_OUTPUT_ATTACHMENT rather than WGPUTextureUsageFlags_OUTPUT_ATTACHMENT in case you have some weirdly named types that overlap constants.

[kvark]

Thing is, the current code already produces #define collisions on a much more frequent basis (since the constants don't get prefixed by the owner type names). And I don't think we should have __ as a separate prefix - I would prefer the type name to match with the constant prefix exactly, which makes the search easier. If we wanted WGPU__, we could specify this as an original type prefix.

[IGI-111]

Good point. Searchability is a must.

What about this case:

#[repr(C)]
struct Foo {}

impl Foo {
    const BAR: i32 = 10;
}

const Foo_BAR: f32 = 99.;

const BAR: u32 = 42.;

What should we output here?

The current output is:

#include <stdint.h>
#include <stdlib.h>
#include <stdbool.h>

#define BAR 10

#define Foo_BAR 99

Interestingly enough it doesn't create an explicit collision. The behavior seems to be that the value declared first is the one outputted. If you change the order of statements, BAR will be 42 and not 10.

So what should we do here? Keep the current behavior? It certainly produces valid output.

[IGI-111]

Verbose mode also indicates the constants generated as INFO which is quite neat. But I think when there is an explicit constant conflict we should WARN.

That feels to me like the least surprising behavior: output a valid header with no explicit conflicts but warn the user that they're most likely doing something wrong.

[kvark]

I believe the probability of a Rust program defining Foo_BAR is extremely low, since it's not even complying with the default naming convention. Thus, we should be safe assuming Foo_BAR is the designated reflection of Foo::BAR constant, and signal otherwise.

Speaking of signalling, I think a collision should be error!, not warn!. Having stuff colliding more often than not would mean a problem for any user of the C header.

[IGI-111]

That's correct. If the standard is followed even acronym types should be CamelCase.

I guess it still leaves single character types and anyone who doesn't follow the naming convention, but point taken those are super rare cases.

Oh and I agree, collisions in this case should be errors.

I'll try to follow this up with a PR tomorrow, the changes should be relatively straightforward.

[kvark]

Awesome, thank you!!!

…

On Oct 23, 2018, at 19:19, IGI-111 ***@***.***> wrote: That's correct. If the standard is followed even acronym types should be CamelCase. I guess it still leaves single character types and anyone who doesn't follow the naming convention, but point taken those are super rare cases. Oh and I agree, collisions in this case should be errors. I'll try to follow this up with a PR tomorrow, the changes should be relatively straightforward. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[IGI-111]

I suppose this ought to be closed now @kvark, I guess the autoclosing didn't work when Github was having hiccups.

[kvark]

@eqrion when is this going to be published?