Skip to content

Commit

Permalink
Small steps towards enabling -Wsign-conversion for some submodules (p…
Browse files Browse the repository at this point in the history 
…onylang#3413)

* libponyc: Be more explicit about return std::move(error)

Doing `return std::move(value)` is, in general, not a good idea, and GCC
warns about it.

```
src/libponyc/codegen/genjit.cc: In lambda function:
src/libponyc/codegen/genjit.cc:46:34: error: redundant move in return
statement [-Werror=redundant-move]
      46 |         if (err) return std::move(err);
	 |                         ~~~~~~~~~^~~~~
src/libponyc/codegen/genjit.cc:46:34: note: remove ‘std::move’ call
```

But in this case, this warning is a false-positive.  What's happening is
an implicit conversion from `Error &&` to `JITSymbol`. Changing the code
to be explicit rather than implicit fixes the warning and makes the code
more clear.

* benchmark: Make all sign conversions explicit

* test: Make all sign conversions explicit

* Explicitly cast list length to a signed type to sum with negative

Nothing in the generated code will change duo this (see
https://godbolt.org/z/n6Dkdp), but it makes it more clear what's
happening and doesn't violate -Wsign-conversion (which I plan to
enable). Without this change, clang reports:

    src/libponyrt/ds/list.c:52:39: error: implicit conversion changes signedness:
    'unsigned long' to 'ssize_t' (aka 'long') [-Werror,-Wsign-conversion]
        index = ponyint_list_length(list) + index;
              ~ ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
    src/libponyrt/ds/list.c:52:41: error: implicit conversion changes signedness:
    'ssize_t' (aka 'long') to 'unsigned long' [-Werror,-Wsign-conversion]
        index = ponyint_list_length(list) + index;
                                          ~ ^~~~~

* Cast the result of __builtin_* calls to unsigned types

The GCC/clang builtins listed here return int even though they never
return a negative number.

The __pony_* wrappers of these builtins all return unsigned and most
of the use-cases of the builtins make sense with the unsigned result,
so I think it makes sense to have unsigned return as the default for
all platforms.

This change means we don't have to add many explicit casts in the calls
to __pony_* to fix -Wsign-conversion violations.
  • Loading branch information
@felipecrv @SeanTAllen
felipecrv authored and SeanTAllen committed Dec 17, 2019
1 parent 86f030a commit bfc9b86
Show file tree
Hide file tree
Showing 10 changed files with 51 additions and 51 deletions.
6 changes: 3 additions & 3 deletions benchmark/libponyc/common/common.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ static void $$$$$$_PauseResumeOverheadOnce(benchmark::State& st) {
st.PauseTiming();
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK($$$$$$_PauseResumeOverheadOnce);
Expand All @@ -17,7 +17,7 @@ static void $$$$$$_PauseResumeOverheadTwice(benchmark::State& st) {
st.PauseTiming();
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK($$$$$$_PauseResumeOverheadTwice);
Expand All @@ -31,7 +31,7 @@ static void $$$$$$_PauseResumeOverheadThrice(benchmark::State& st) {
st.PauseTiming();
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK($$$$$$_PauseResumeOverheadThrice);
6 changes: 3 additions & 3 deletions benchmark/libponyrt/common/common.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ static void $$$$$$_PauseResumeOverheadOnce(benchmark::State& st) {
st.PauseTiming();
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK($$$$$$_PauseResumeOverheadOnce);
Expand All @@ -17,7 +17,7 @@ static void $$$$$$_PauseResumeOverheadTwice(benchmark::State& st) {
st.PauseTiming();
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK($$$$$$_PauseResumeOverheadTwice);
Expand All @@ -31,7 +31,7 @@ static void $$$$$$_PauseResumeOverheadThrice(benchmark::State& st) {
st.PauseTiming();
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK($$$$$$_PauseResumeOverheadThrice);
32 changes: 16 additions & 16 deletions benchmark/libponyrt/ds/hash.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ void HashMapBench::delete_elements(size_t del_pct, size_t count)
// delete random items until map size is as small as required
while(testmap_size(&_map) > final_count)
{
e1->key = rand() % count;
e1->key = (size_t)rand() % count;

hash_elem_t* d = testmap_remove(&_map, e1);
if(d != NULL)
Expand Down Expand Up @@ -132,7 +132,7 @@ BENCHMARK_DEFINE_F(HashMapBench, HashDestroy$)(benchmark::State& st) {
testmap_destroy(&_map);
}
testmap_init(&_map, sz);
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(HashMapBench, HashDestroy$)->RangeMultiplier(2)->Ranges({{1, 32<<10}, {0, 0}});
Expand Down Expand Up @@ -164,7 +164,7 @@ BENCHMARK_DEFINE_F(HashMapBench, HashResize$)(benchmark::State& st) {
st.ResumeTiming();
testmap_put(&_map, curr);
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(HashMapBench, HashResize$)->RangeMultiplier(2)->Ranges({{1, 32<<10}, {0, 0}});
Expand All @@ -187,7 +187,7 @@ BENCHMARK_DEFINE_F(HashMapBench, HashNext)(benchmark::State& st) {
break; // Needed to skip the rest of the iteration.
}
}
st.SetItemsProcessed(st.iterations() * (testmap_size(&_map) + 1));
st.SetItemsProcessed((int64_t)(st.iterations() * (testmap_size(&_map) + 1)));
}

BENCHMARK_REGISTER_F(HashMapBench, HashNext)->RangeMultiplier(2)->Ranges({{1, 32<<10}, {1, 32}});
Expand All @@ -204,7 +204,7 @@ BENCHMARK_DEFINE_F(HashMapBench, HashPut)(benchmark::State& st) {
testmap_put(&_map, curr);
i++;
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
for(i = 0; i < _map.contents.size; i++)
testmap_clearindex(&_map, i);
free_elem(curr);
Expand All @@ -222,7 +222,7 @@ BENCHMARK_DEFINE_F(HashMapBench, HashPutResize)(benchmark::State& st) {
testmap_put(&_map, curr);
i++;
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
for(i = 0; i < _map.contents.size; i++)
testmap_clearindex(&_map, i);
free_elem(curr);
Expand All @@ -245,7 +245,7 @@ BENCHMARK_DEFINE_F(HashMapBench, HashPutNew)(benchmark::State& st) {
}
if(curr != NULL)
free_elem(curr);
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(HashMapBench, HashPutNew)->RangeMultiplier(2)->Ranges({{32<<10, 32<<10}, {0, 0}});
Expand All @@ -261,7 +261,7 @@ BENCHMARK_DEFINE_F(HashMapBench, HashPutNewResize)(benchmark::State& st) {
testmap_put(&_map, curr);
i++;
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(HashMapBench, HashPutNewResize)->RangeMultiplier(2)->Ranges({{1, 1}, {0, 0}});
Expand All @@ -288,7 +288,7 @@ BENCHMARK_DEFINE_F(HashMapBench, HashRemove$_)(benchmark::State& st) {
testmap_remove(&_map, curr);
}
}
st.SetItemsProcessed(st.iterations() * sz);
st.SetItemsProcessed((int64_t)(st.iterations() * sz));
for(size_t i = 0; i < sz; i++)
free_elem(a[i]);
free_elem(curr);
Expand All @@ -314,14 +314,14 @@ BENCHMARK_DEFINE_F(HashMapBench, HashSearch)(benchmark::State& st) {
size_t t = 0;
size_t r = 0;
for(size_t i = 0; i < map_count; i++) {
r = rand() & map_count_mask;
r = (size_t)rand() & map_count_mask;
t = a[r];
a[r] = a[i];
a[i] = t;
}
} else {
for(size_t i = 0; i < map_count; i++) {
a[i] = rand() + incr;
a[i] = (size_t)rand() + incr;
}
}

Expand All @@ -332,7 +332,7 @@ BENCHMARK_DEFINE_F(HashMapBench, HashSearch)(benchmark::State& st) {
testmap_get(&_map, e1, &index);
j++;
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
ponyint_pool_free_size(map_count, a);
free_elem(e1);
e1 = nullptr;
Expand Down Expand Up @@ -378,20 +378,20 @@ BENCHMARK_DEFINE_F(HashMapBench, HashSearchDeletes)(benchmark::State& st) {
a[i] = n->key;
}
} else {
a[i] = a[rand() % map_count];
a[i] = a[(size_t)rand() % map_count];
}
}
size_t t = 0;
size_t r = 0;
for(size_t i = 0; i < array_size; i++) {
r = rand() & array_size_mask;
r = (size_t)rand() & array_size_mask;
t = a[r];
a[r] = a[i];
a[i] = t;
}
} else {
for(size_t i = 0; i < array_size; i++) {
a[i] = rand() + count;
a[i] = (size_t)rand() + count;
}
}

Expand All @@ -402,7 +402,7 @@ BENCHMARK_DEFINE_F(HashMapBench, HashSearchDeletes)(benchmark::State& st) {
testmap_get(&_map, e1, &ind);
j++;
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
ponyint_pool_free_size(array_size, a);
free_elem(e1);
e1 = nullptr;
Expand Down
10 changes: 5 additions & 5 deletions benchmark/libponyrt/mem/heap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ BENCHMARK_DEFINE_F(HeapBench, HeapAlloc$)(benchmark::State& st) {
ponyint_heap_init(&_heap);
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(HeapBench, HeapAlloc$)->RangeMultiplier(2)->Ranges({{32, 1024<<10}});
Expand All @@ -68,7 +68,7 @@ BENCHMARK_DEFINE_F(HeapBench, HeapAlloc$_)(benchmark::State& st) {
ponyint_heap_init(&_heap);
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations()*num_allocs);
st.SetItemsProcessed((int64_t)st.iterations() * num_allocs);
}

BENCHMARK_REGISTER_F(HeapBench, HeapAlloc$_)->RangeMultiplier(2)->Ranges({{32, 1024<<10}, {1<<10, 1<<10}});
Expand All @@ -88,7 +88,7 @@ BENCHMARK_DEFINE_F(HeapBench, HeapDestroy$)(benchmark::State& st) {
st.ResumeTiming();
ponyint_heap_destroy(&_heap);
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
ponyint_heap_init(&_heap);
}

Expand All @@ -112,7 +112,7 @@ BENCHMARK_DEFINE_F(HeapBench, HeapDestroyMultiple$)(benchmark::State& st) {
st.ResumeTiming();
ponyint_heap_destroy(&_heap);
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
ponyint_heap_init(&_heap);
}

Expand All @@ -131,7 +131,7 @@ BENCHMARK_DEFINE_F(HeapBench, HeapInitAllocDestroy)(benchmark::State& st) {
ponyint_heap_alloc_small(actor, &_heap, ponyint_heap_index(alloc_size));
ponyint_heap_destroy(&_heap);
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
ponyint_heap_init(&_heap);
}

Expand Down
26 changes: 13 additions & 13 deletions benchmark/libponyrt/mem/pool.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ BENCHMARK_DEFINE_F(PoolBench, pool_index)(benchmark::State& st) {
while (st.KeepRunning()) {
ponyint_pool_index(static_cast<size_t>(st.range(0)));
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(PoolBench, pool_index)->RangeMultiplier(3)->Ranges({{1, 1024<<10}});
Expand All @@ -46,7 +46,7 @@ BENCHMARK_DEFINE_F(PoolBench, POOL_ALLOC$)(benchmark::State& st) {
POOL_FREE(block_t, p);
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(PoolBench, POOL_ALLOC$);
Expand All @@ -62,7 +62,7 @@ BENCHMARK_DEFINE_F(PoolBench, POOL_ALLOC_multiple$_)(benchmark::State& st) {
POOL_FREE(block_t, p[i-1]);
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations()*num_allocs);
st.SetItemsProcessed((int64_t)(st.iterations() * num_allocs));
}

BENCHMARK_REGISTER_F(PoolBench, POOL_ALLOC_multiple$_)->Arg(1<<10);
Expand All @@ -74,7 +74,7 @@ BENCHMARK_DEFINE_F(PoolBench, POOL_FREE$)(benchmark::State& st) {
st.ResumeTiming();
POOL_FREE(block_t, p);
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(PoolBench, POOL_FREE$);
Expand All @@ -84,7 +84,7 @@ BENCHMARK_DEFINE_F(PoolBench, POOL_ALLOC_FREE)(benchmark::State& st) {
void* p = POOL_ALLOC(block_t);
POOL_FREE(block_t, p);
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(PoolBench, POOL_ALLOC_FREE);
Expand All @@ -100,7 +100,7 @@ BENCHMARK_DEFINE_F(PoolBench, POOL_FREE_multiple$_)(benchmark::State& st) {
for(size_t i = num_allocs; i > 0; i--)
POOL_FREE(block_t, p[i-1]);
}
st.SetItemsProcessed(st.iterations()*num_allocs);
st.SetItemsProcessed((int64_t)(st.iterations() * num_allocs));
}

BENCHMARK_REGISTER_F(PoolBench, POOL_FREE_multiple$_)->Arg(1<<10);
Expand All @@ -114,7 +114,7 @@ BENCHMARK_DEFINE_F(PoolBench, POOL_ALLOC_FREE_multiple)(benchmark::State& st) {
for(size_t i = num_allocs; i > 0; i--)
POOL_FREE(block_t, p[i-1]);
}
st.SetItemsProcessed(st.iterations()*num_allocs);
st.SetItemsProcessed((int64_t)(st.iterations() * num_allocs));
}

BENCHMARK_REGISTER_F(PoolBench, POOL_ALLOC_FREE_multiple)->Arg(1<<10);
Expand All @@ -126,7 +126,7 @@ BENCHMARK_DEFINE_F(PoolBench, pool_alloc_size$)(benchmark::State& st) {
ponyint_pool_free_size(LARGE_ALLOC, p);
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(PoolBench, pool_alloc_size$);
Expand All @@ -142,7 +142,7 @@ BENCHMARK_DEFINE_F(PoolBench, pool_alloc_size_multiple$_)(benchmark::State& st)
ponyint_pool_free_size(LARGE_ALLOC, p[i-1]);
st.ResumeTiming();
}
st.SetItemsProcessed(st.iterations()*num_allocs);
st.SetItemsProcessed((int64_t)(st.iterations() * num_allocs));
}

BENCHMARK_REGISTER_F(PoolBench, pool_alloc_size_multiple$_)->Arg((int)(POOL_MMAP/LARGE_ALLOC));
Expand All @@ -154,7 +154,7 @@ BENCHMARK_DEFINE_F(PoolBench, pool_free_size$)(benchmark::State& st) {
st.ResumeTiming();
ponyint_pool_free_size(LARGE_ALLOC, p);
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(PoolBench, pool_free_size$);
Expand All @@ -164,7 +164,7 @@ BENCHMARK_DEFINE_F(PoolBench, pool_alloc_free_size)(benchmark::State& st) {
void* p = ponyint_pool_alloc_size(LARGE_ALLOC);
ponyint_pool_free_size(LARGE_ALLOC, p);
}
st.SetItemsProcessed(st.iterations());
st.SetItemsProcessed((int64_t)st.iterations());
}

BENCHMARK_REGISTER_F(PoolBench, pool_alloc_free_size);
Expand All @@ -180,7 +180,7 @@ BENCHMARK_DEFINE_F(PoolBench, pool_free_size_multiple$_)(benchmark::State& st) {
for(size_t i = num_allocs; i > 0; i--)
ponyint_pool_free_size(LARGE_ALLOC, p[i-1]);
}
st.SetItemsProcessed(st.iterations()*num_allocs);
st.SetItemsProcessed((int64_t)(st.iterations() * num_allocs));
}

BENCHMARK_REGISTER_F(PoolBench, pool_free_size_multiple$_)->Arg((int)(POOL_MMAP/LARGE_ALLOC));
Expand All @@ -194,7 +194,7 @@ BENCHMARK_DEFINE_F(PoolBench, pool_alloc_free_size_multiple)(benchmark::State& s
for(size_t i = num_allocs; i > 0; i--)
ponyint_pool_free_size(LARGE_ALLOC, p[i-1]);
}
st.SetItemsProcessed(st.iterations()*num_allocs);
st.SetItemsProcessed((int64_t)(st.iterations() * num_allocs));
}

BENCHMARK_REGISTER_F(PoolBench, pool_alloc_free_size_multiple)->Arg((int)(POOL_MMAP/LARGE_ALLOC));
14 changes: 7 additions & 7 deletions src/common/platform.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,13 +232,13 @@ inline int snprintf(char* str, size_t size, const char* format, ...)
*
*/
#ifdef PLATFORM_IS_CLANG_OR_GCC
# define __pony_popcount(X) __builtin_popcount((X))
# define __pony_ffs(X) __builtin_ffs((X))
# define __pony_ffsl(X) __builtin_ffsl((X))
# define __pony_ctz(X) __builtin_ctz((X))
# define __pony_ctzl(X) __builtin_ctzl((X))
# define __pony_clz(X) __builtin_clz((X))
# define __pony_clzl(X) __builtin_clzl((X))
# define __pony_popcount(X) (unsigned int)__builtin_popcount((X))
# define __pony_ffs(X) (unsigned int)__builtin_ffs((X))
# define __pony_ffsl(X) (unsigned int)__builtin_ffsl((X))
# define __pony_ctz(X) (unsigned int)__builtin_ctz((X))
# define __pony_ctzl(X) (unsigned int)__builtin_ctzl((X))
# define __pony_clz(X) (unsigned int)__builtin_clz((X))
# define __pony_clzl(X) (unsigned int)__builtin_clzl((X))
#else
# include <intrin.h>
# define __pony_popcount(X) __popcnt((X))
Expand Down
2 changes: 1 addition & 1 deletion src/libponyc/codegen/genjit.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class PonyJIT
if (symbol) return symbol;

auto err = symbol.takeError();
if (err) return std::move(err);
if (err) return JITSymbol(std::move(err));

auto symaddr = RTDyldMemoryManager::getSymbolAddressInProcess(name);
if (symaddr)
Expand Down
2 changes: 1 addition & 1 deletion src/libponyrt/ds/list.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ list_t* ponyint_list_next(list_t* list)
list_t* ponyint_list_index(list_t* list, ssize_t index)
{
if(index < 0)
index = ponyint_list_length(list) + index;
index = (ssize_t)ponyint_list_length(list) + index;

for(int i = 0; (list != NULL) && (i < index); i++)
list = list->next;
Expand Down
Loading

0 comments on commit bfc9b86

Please sign in to comment.