Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate Clone Functions v 7.0.2 #354

Open
Astn opened this issue Nov 26, 2022 · 4 comments
Open

Duplicate Clone Functions v 7.0.2 #354

Astn opened this issue Nov 26, 2022 · 4 comments

Comments

@Astn
Copy link

Astn commented Nov 26, 2022

Getting duplicate clone helpers generated with the latest version.

Sample.fbs

attribute "fs_serializer";
attribute "fs_vector";
attribute "fs_valueStruct";
attribute "fs_unsafeExternal";

namespace DuplicateCloneHelpers;

struct B     (fs_valueStruct, fs_unsafeExternal:"System.Boolean") { data: bool; }
struct I8    (fs_valueStruct, fs_unsafeExternal:"System.SByte") { data: int8;}


union Primitive {
     B,
     I8,
}

Snip from FlatSharp.generated.cs showing the duplication.

namespace FlatSharp.Compiler.Generated
{
    internal static class CloneHelpers_011a996105fe4ccfbd87a7227c98bbdb
    {
        [global::System.Runtime.CompilerServices.MethodImplAttribute(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
        public static global::System.Boolean Clone(global::System.Boolean item)
        {
            checked
            {
                return item;
            }
        }

        [global::System.Runtime.CompilerServices.MethodImplAttribute(System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)]
        public static global::System.Boolean Clone(global::System.Boolean item)
        {
            checked
            {
                return item;
            }
        }
        ...
    }
}
@jamescourtney
Copy link
Owner

I can see how that would happen. What has possessed you to redefine the existing primitives? unsafeExternal was really intended for SIMD-accelerated types or other things FlatSharp doesn't support out of the box.

@jamescourtney
Copy link
Owner

Oh, I see -- You're trying to get around the FlatBuffer Union restrictions by mapping primitive types externally. That's very clever and not something that I anticipated at all.

Clone methods are implemented "on the side" and work by overloading the "Clone" method so that the generated code doesn't have to be very smart. There's a Clone method available for every type in the object graph. For the dumb ones like primitives, strings, and value structs, they are just annotated with [MethodImpl(MethodImplOptions.AggressiveInlining)] so that it's functionally a copy. Since FlatSharp can't really resolve external types, it just knows that they are value structs so the Clone method gets implemented twice as you see.

The cheapest possible fix here is to just not emit clone methods at all for value structs since they are values. However, I need to actually revisit copy constructors wholesale at some point to remove some extra allocations. Let me noodle on this for a few days and get back to you on this thread.

Thanks for raising the issue!

@Astn
Copy link
Author

Astn commented Nov 27, 2022

In the interim I have just copied the generated file out of the obj folder and removed the duplicate function definitions. It's working great so far with that workaround.

@jamescourtney
Copy link
Owner

Just FYI -- I'm not in a big hurry to fix this. It seems like the workaround is pretty easy and this seems to be a pretty niche scenario, so I'm going to take the time and make sure I can do it correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants