[C++ification] Making AndroidSystem class more C++ and faster by grendello · Pull Request #2599 · dotnet/android

grendello · 2019-01-09T18:08:00Z

It's a start of effort to slowly (and piece by piece) modify our runtime code
away from the C syntax toward the C++ one, using idioms and constructs from the
latter wherever possible, practical or simply better. The goal is to have code
that's safer, more correct, faster as well as to expose only the minimum
"public" API of every class in the interest of minimizing the potential of code
abuse.

At the same time, the changes should not affect the way the code works on the
runtime (unless the change implements a performance increase)

Each commit/PR in this series will attempt to mimimize collateral changes to
other classes/parts of code, but sometimes (as in this case) there will be
minimal changes in other areas related to the main class changed by a commit.

The following changes to the AndroidSystem class are made in this commit:

The BundledProperty structure moves to android-system.cc - it has no use
outside the class
Several printf family calls are replaced with either simple strdup or by
compile-time string concatenation.
for loops use size_t type for the counter variable
nullptr is used in place of the !pointer or pointer in conditional
expressions or asserts.
The _monodroid__system_property_get overload which uses popen(3) to
retrieve the property is removed - the __system_property_get call is still
present in the Android NDK (albeit deprecated) and since we're compiling
against API16 at least, we're in no immediate danger of missing that call
Local function variables are moved closer to places where they are first
initialized/used.
Replaced the TRY_LIBMONOSGEN macro with an inline function
Replaced use of mono_bool with the C++ type bool
Replaced a number of malloc/free calls with new/delete
The MONO_SGEN_ARCH_SO constant is now generated on the compile time
instead of on the runtime.

Other changes:

Define the ANDROID64 macro on build time if targetting 64-bit Android.
This macro was mistakenly omitted in conversion to cmake
Fix a handful of warnings regarding our new/delete operator
implementations.

grendello · 2019-01-10T07:06:08Z

build

grendello · 2019-01-10T17:57:52Z

build

grendello · 2019-01-10T21:21:52Z

@jonpryor missed that struct :)

Depends on: dotnet#2599 * 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

jonpryor · 2019-01-11T13:37:20Z

+ } else if (utils.monodroid_dirent_hasextension (e, ".jm")) {
 if (!add_type_mapping (&java_to_managed_maps, file_path, nullptr, ((const char*)val)))
-free (val);
+delete val;


This should be delete[] val, because new[] is used to allocate it within monodroid_read_file_into_memory(), e.g. https://github.com/xamarin/xamarin-android/pull/2599/files#diff-85c855eb9ec814df87fe0f2bf21b9311R219
Suggested change
delete val;
delete[] val;

jonpryor · 2019-01-11T13:45:57Z

- free (val);
- }
- if (utils.monodroid_dirent_hasextension (e, ".jm")) {
+ delete val;


Ditto here:
Suggested change
delete val;
delete[] val;

jonpryor · 2019-01-11T13:47:53Z

 	if (result > 0) {
 if (strlen (local_value) == 0) {
-free (local_value);
+delete local_value;


This should be delete[] local_value.

jonpryor · 2019-01-11T13:47:59Z

 *value = local_value;
 else
-free (local_value);
+delete local_value;


This should be delete[] local_value.

jonpryor · 2019-01-11T13:49:10Z

Given all the delete-vs-delete[] "typos"/mistakes I just discovered, I wonder if instead of s/free/delete we should instead "more fully embrace C++" and just introduce a damn string type.

jonpryor · 2019-01-11T13:53:10Z

+	if (buf != nullptr) {
 strncpy (sp_value, buf, sp_value_len);
 sp_value [sp_value_len] = '\0';
 delete buf;


...and this was overlooked previously; this should be delete[] buf.

It's a start of effort to slowly (and piece by piece) modify our runtime code away from the C syntax toward the C++ one, using idioms and constructs from the latter wherever possible, practical or simply better. The goal is to have code that's safer, more correct, faster as well as to expose only the minimum "public" API of every class in the interest of minimizing the potential of code abuse. At the same time, the changes should not affect the way the code works on the runtime (unless the change implements a performance increase) Each commit/PR in this series will attempt to mimimize collateral changes to other classes/parts of code, but sometimes (as in this case) there will be minimal changes in other areas related to the main class changed by a commit. The following changes to the `AndroidSystem` class are made in this commit: * The `BundledProperty` structure moves to `android-system.cc` - it has no use outside the class * Several `printf` family calls are replaced with either simple `strdup` or by compile-time string concatenation. * `for` loops use `size_t` type for the counter variable * `nullptr` is used in place of the `!pointer` or `pointer` in conditional expressions or asserts. * The `_monodroid__system_property_get` overload which uses `popen(3)` to retrieve the property is removed - the `__system_property_get` call is still present in the Android NDK (albeit deprecated) and since we're compiling against API16 at least, we're in no immediate danger of missing that call * Local function variables are moved closer to places where they are first initialized/used. * Replaced the `TRY_LIBMONOSGEN` macro with an inline function * Replaced use of `mono_bool` with the C++ type `bool` * Replaced a number of `malloc/free` calls with `new/delete` * The `MONO_SGEN_ARCH_SO` constant is now generated on the compile time instead of on the runtime. Other changes: * Define the `ANDROID64` macro on build time if targetting 64-bit Android. This macro was mistakenly omitted in conversion to `cmake` * Fix a handful of warnings regarding our `new/delete` operator implementations.

Depends on: dotnet#2599 * 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 requested a review from jonpryor as a code owner January 9, 2019 18:08

jonpryor added the full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps) label Jan 9, 2019

grendello force-pushed the cppify branch from dccc296 to c6522a3Compare January 9, 2019 18:29

grendello mentioned this pull request Jan 10, 2019
[C++-ification] Introducing EmbeddedAssemblies class #2606
Merged

jonpryor reviewed Jan 11, 2019
View reviewed changes

grendello force-pushed the cppify branch from ecbcc3f to 53b6af7Compare January 11, 2019 16:59

jonpryor merged commit 6c4939d into dotnet:masterJan 11, 2019

grendello deleted the cppify branch February 13, 2019 08:59

github-actionsBot locked and limited conversation to collaborators Feb 1, 2024

Navigation Menu

Uh oh!

[C++ification] Making AndroidSystem class more C++ and faster - #2599

[C++ification] Making AndroidSystem class more C++ and faster#2599
jonpryor merged 1 commit into
dotnet:masterfrom
grendello:cppify

grendello commented Jan 9, 2019

Uh oh!

grendello commented Jan 10, 2019

Uh oh!

grendello commented Jan 10, 2019

Uh oh!

grendello commented Jan 10, 2019

Uh oh!

jonpryorJan 11, 2019

Uh oh!

jonpryorJan 11, 2019

Uh oh!

jonpryorJan 11, 2019

Uh oh!

jonpryorJan 11, 2019

Uh oh!

jonpryor commented Jan 11, 2019

Uh oh!

jonpryorJan 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

grendello commented Jan 9, 2019

Uh oh!

grendello commented Jan 10, 2019

Uh oh!

grendello commented Jan 10, 2019

Uh oh!

grendello commented Jan 10, 2019

Uh oh!

jonpryorJan 11, 2019

Choose a reason for hiding this comment

Uh oh!

jonpryorJan 11, 2019

Choose a reason for hiding this comment

Uh oh!

jonpryorJan 11, 2019

Choose a reason for hiding this comment

Uh oh!

jonpryorJan 11, 2019

Choose a reason for hiding this comment

Uh oh!

jonpryor commented Jan 11, 2019

Uh oh!

jonpryorJan 11, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants