search

ARMmbed / core-util

DEPRECATED: Mbed 3 utilities library
Other
12 stars 17 forks source link

Fix issue#54 #85

Closed 0xc0170 closed 8 years ago

0xc0170 commented 8 years ago 
+---------------+---------------+--------------------------+--------+--------------------+-------------+
| target        | platform_name | test                     | result | elapsed_time (sec) | copy_method |
+---------------+---------------+--------------------------+--------+--------------------+-------------+
| frdm-k64f-gcc | K64F          | core-util-test-sbrk-mini | OK     | 2.39               | shell       |
+---------------+---------------+--------------------------+--------+--------------------+-------------+

This change-set shall be merged after #83, as that one fixes the internal tests. Then this one should not cause any failure.

I would like to eliminate those 2 TODOs in PoolAllocator. I can just remove them, any suggestions?

@bremoran @mjs-arm @bogdanm

0xc0170 commented 8 years ago

It's rebased on top of master, tests below run with this changeset:

+---------------+---------------+--------------------------+--------+--------------------+-------------+
| target        | platform_name | test                     | result | elapsed_time (sec) | copy_method |
+---------------+---------------+--------------------------+--------+--------------------+-------------+
| frdm-k64f-gcc | K64F          | core-util-test-test-krbs | OK     | 4.02               | shell       |
+---------------+---------------+--------------------------+--------+--------------------+-------------+

+---------------+---------------+--------------------------+--------+--------------------+-------------+
| target        | platform_name | test                     | result | elapsed_time (sec) | copy_method |
+---------------+---------------+--------------------------+--------+--------------------+-------------+
| frdm-k64f-gcc | K64F          | core-util-test-test-sbrk | OK     | 4.02               | shell       |
+---------------+---------------+--------------------------+--------+--------------------+-------------+
0xc0170 commented 8 years ago

Bump for review

bremoran commented 8 years ago

I would suggest eliminating the TODOs using yotta config with sensible defaults.

#ifndef YOTTA_CFG_CORE_UTIL_POOL_ALLOC_DEFAULT_ALIGN
#define YOTTA_CFG_CORE_UTIL_POOL_ALLOC_DEFAULT_ALIGN 8
#endif 
#define MBED_UTIL_POOL_ALLOC_DEFAULT_ALIGN YOTTA_CFG_CORE_UTIL_POOL_ALLOC_DEFAULT_ALIGN

Please delete upload.tar.gz

0xc0170 commented 8 years ago

Please delete upload.tar.gz

Ah, was not ignored. I'll send a fix for .gitignore in the separate PR, file has been removed.

0xc0170 commented 8 years ago 
#ifndef YOTTA_CFG_CORE_UTIL_POOL_ALLOC_DEFAULT_ALIGN
#define YOTTA_CFG_CORE_UTIL_POOL_ALLOC_DEFAULT_ALIGN 8
#endif 
#define MBED_UTIL_POOL_ALLOC_DEFAULT_ALIGN YOTTA_CFG_CORE_UTIL_POOL_ALLOC_DEFAULT_ALIGN

Sensible to me, anyone else any objections?

ghost commented 8 years ago

LGTM

bremoran commented 8 years ago

LGTM

0xc0170 commented 8 years ago

I eliminated those TODO as suggested above with yotta config. Ready for merging

bogdanm commented 8 years ago

Ready for merging, I agree. So:

  1. because we increased the alignment, this fix should be functionally safe, which means that we can release it as a patch.
  2. however, also because we increased the alignment, this fix might lead to regressions caused by increased RAM usage.

Because of 2 above, I'm inclined to release this fix as a new major version (which of course means that we'll need to update all the dependants accordingly). Thoughts? @bremoran @0xc0170

bogdanm commented 8 years ago

After a few discussion, it looks like the best option is to look at this like a bugfix (which it actually is) and thus release a new patch version once it's merged.

bogdanm commented 8 years ago

Published version 1.5.2