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

[C++-ification] Introducing EmbeddedAssemblies class #2606

Merged
merged 1 commit into from Jan 14, 2019

Conversation

Projects
None yet
3 participants
@grendello
Copy link
Contributor

grendello commented Jan 10, 2019

  • removed a number of MONO_API functions, they don't appear to be used anywhere (were supposed to be used by Mono Unreal Engine, but we're not sure if that's the case). We can restore them if needed
  • Simplified a lot of code
  • Removed printf family calls

@grendello grendello requested a review from jonpryor as a code owner Jan 10, 2019

@grendello grendello force-pushed the grendello:cppify+1 branch from 2598bf5 to f2bea65 Jan 11, 2019

@grendello grendello changed the title [WIP, C++-ification] Introducing EmbeddedAssemblies class [C++-ification] Introducing EmbeddedAssemblies class Jan 11, 2019

@grendello grendello force-pushed the grendello:cppify+1 branch from f2bea65 to 7c7c86d Jan 11, 2019

int entry_length;
int value_offset;
const char *mapping;
struct TypeMappingInfo *next;

This comment has been minimized.

@jonpryor

jonpryor Jan 11, 2019

Contributor

Yet another struct StructType which could instead be StructType ;-)

}

MONO_API int
monodroid_embedded_assemblies_set_assemblies_prefix (const char *prefix)

This comment has been minimized.

@jonpryor

jonpryor Jan 11, 2019

Contributor

There is Another Repo that cares about our "public API" surface: https://github.com/mono/Embeddinator-4000

In particular, they call monodroid_embedded_assemblies_set_assemblies_prefix():

https://github.com/mono/Embeddinator-4000/blob/c237e21b63df40799ef251350ee961846c549553/support/mono_embeddinator.c#L62

As such:

  1. This export shouldn't be removed, and
  2. I don't think EmbeddedAssemblies::assemblies_prefix can actually be a constexpr.

This comment has been minimized.

@grendello

grendello Jan 11, 2019

Contributor
  1. is it just this API or all of the ones which were here before?
  2. it can - it's the default, we can use it as long as the 3rd party sets a new one, in which case we'll use the new value

This comment has been minimized.

@jonpryor

jonpryor Jan 11, 2019

Contributor

I suppose I don't understand constexpr then; I thought constexpr was for compile-time constants.

@jonathanpeppers: Do you have a feel for which parts of our libmonodroid.so API Embeddinator-4000 uses?

@grendello: From what I can easily tell, monodroid_embedded_assemblies_set_assemblies_prefix is the only one:

$ git grep monodroid_
support/mono_embeddinator.c:MONO_API int monodroid_embedded_assemblies_set_assemblies_prefix (const char *prefix);
support/mono_embeddinator.c:    monodroid_embedded_assemblies_set_assemblies_prefix("assets/assemblies/");

Looking at git grep mono...they use mono.android.jar?!

/me backs away slowly and awaits @jonathanpeppers response.

This comment has been minimized.

@grendello

grendello Jan 11, 2019

Contributor

@jonpryor you understand constexpr correctly, but see the updated code here. We should optimize for the most common case - and that is our prefix in this instance.

This comment has been minimized.

@jonathanpeppers

jonathanpeppers Jan 11, 2019

Contributor

monodroid_embedded_assemblies_set_assemblies_prefix is the only one called by Embeddinator.

I was not able to put assembly files in /assemblies, because AAR files couldn't have arbitrary directories like that. I put them in assets/assemblies and called monodroid_embedded_assemblies_set_assemblies_prefix. I had to include instructions to leave .dll files uncompressed.

size_t si;

MonoAssembly *a = nullptr;
const char *culture = reinterpret_cast<const char*> (monoFunctions.assembly_name_get_culture (aname));

This comment has been minimized.

@jonpryor

jonpryor Jan 11, 2019

Contributor

I suppose I should have asked this ages ago, but why monoFunctions and not mono?

This comment has been minimized.

@jonpryor

jonpryor Jan 11, 2019

Contributor

Why is this reinterpret_cast<const char*> needed in the first place? DylibMono::assembly_name_get_culture() already returns const char*. This cast should not be needed.

This comment has been minimized.

@grendello

grendello Jan 11, 2019

Contributor

because it's not Mono, just its functions :)


MonoAssembly *a = nullptr;
const char *culture = reinterpret_cast<const char*> (monoFunctions.assembly_name_get_culture (aname));
const char *asmname = reinterpret_cast<const char*> (monoFunctions.assembly_name_get_name (aname));

This comment has been minimized.

@jonpryor

jonpryor Jan 11, 2019

Contributor

Ditto here:

const char*
DylibMono::assembly_name_get_name (MonoAssemblyName *aname)

DylibMono::assembly_name_get_name() already returns a const char*. Why is there a cast?

@@ -156,11 +135,10 @@ monodroid_typemap_java_to_managed (const char *java)
return nullptr;
}

MONO_API const char *
monodroid_typemap_managed_to_java (const char *managed)
inline const char*

This comment has been minimized.

@jonpryor

jonpryor Jan 11, 2019

Contributor

You're inlining a method with a for loop?!

Why?

This comment has been minimized.

@grendello

grendello Jan 11, 2019

Contributor

To make it faster without incurring another call overhead? This way we end up with code identical, performance and size wise, as before.

const char *data = addr;

if (!p)
struct TypeMappingInfo *p = new TypeMappingInfo (); // calloc (1, sizeof (struct TypeMappingInfo));

This comment has been minimized.

@jonpryor

jonpryor Jan 11, 2019

Contributor

Yet another struct that can be removed.

@grendello grendello force-pushed the grendello:cppify+1 branch from 7c7c86d to fed721a Jan 11, 2019


if (!p)
TypeMappingInfo *p = new TypeMappingInfo (); // calloc (1, sizeof (struct TypeMappingInfo));
if (p == nullptr)

This comment has been minimized.

@jonpryor

jonpryor Jan 11, 2019

Contributor

This is now redundant/unnecessary/contradictory/a source of confusion; as per this and related comments:
#2515 (comment)

We have explicitly stated that new T will either return memory, or abort the process. There is thus no need to continue checking for nullptr return values, unless new(nothrow) T is used.

Thus, a philosophical question: should we have "error handling code" for scenarios that we know and guarantee cannot possibly happen? p will NEVER be nullptr; as such, this and the following line are dead.

Worse than dead, though; misleading: their mere presence suggests that perhaps new T can return NULL, which will be confusing to any newcomers to the codebase: some places are checking the return value, some don't, so which is correct? ¯_(ツ)_/¯

...which is yet another thing I missed/overlooked in PR #2599: it also contains places which use new, then check the return value.

We should be consistent.

Furthermore, consistency should be this:

  • If new T is used, do not check the return value. It cannot be NULL.
  • If new(nothrow) T is used, check the return value; it may be NULL.

@grendello grendello force-pushed the grendello:cppify+1 branch from fed721a to f4f7cf9 Jan 11, 2019

should_register = r;
should_register_data = d;
if (assemblies_prefix_override != nullptr)
delete[] assemblies_prefix_override;

This comment has been minimized.

@jonpryor

jonpryor Jan 11, 2019

Contributor

This should be free(assemblies_prefix_override), not delete[], as we use strdup().

This comment has been minimized.

@grendello

grendello Jan 11, 2019

Contributor

This is in preparation for the changes in this PR , it was deliberate

[C++-ification] Introducing EmbeddedAssemblies class
  * removed a number of `MONO_API` functions, they don't appear to be used
    anywhere (were supposed to be used by Mono Unreal Engine, but we're not sure
    if that's the case). We can restore them if needed
  * Simplified a lot of code
  * Removed `printf` family calls

@grendello grendello force-pushed the grendello:cppify+1 branch from f4f7cf9 to 8ecfaca Jan 14, 2019

@jonpryor jonpryor merged commit 434d2d8 into xamarin:master Jan 14, 2019

4 checks passed

Ubuntu PR Build Build finished. No test results found.
Details
license/cla All CLA requirements met.
Details
macOS PR Debug Build Build finished. 86770 tests run, 14 skipped, 0 failed.
Details
macOS PR Release Build Build finished. 172335 tests run, 16 skipped, 0 failed.
Details

@grendello grendello deleted the grendello:cppify+1 branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment