comparing integer expressions of different signedness in memory benchmarking file

[BabarZKhan]

there is not really a 'bug' but in for loop of memtest_parallel.cpp file, the condition in loop is comparing integer expressions of different signedness. This can give compiler error. I think the variable instances can also be declared as int

[sommerlukas]

@BabarZKhan: There are multiple for-loops in memtest_parallel.cpp, which one were you referring to?

Do you maybe want to provide a fix yourself?

[BabarZKhan]

@sommerlukas : thanks for the reply.

Yes, there are multiple for loops. And to be specific there are 11 for loops. And this condition of comparing integer expressions of different signedness i.e. instance < instances happens in 6 for loops. So I am referring to for loops at following lines:

• line 77
• line 91
• line 130
• line 161
• line 172
• line 182

uint64_t instances = tapasco.kernel_pe_count(PE_ID); int instance = 0;

In current code the data type of variable instances is uint64_t. I think the data type of variable instances can be also be simply int. AFAIK, in PEs there will never more than int instances. Or we can simply make instance variable as uint64_t

Yes, I can provide a fix. But please let me know your opinion. I got compiler error but in some compiler this can just be a warning. This is why I mentioned not really a 'bug'. :-)

[sommerlukas]

We should definitely fix this, even if it only is a warning, it is still a "code smell". IMHO, instance should be an unsigned type, too. As the number of instances will never be a negative number (0 in the worst case), there is no actual reason to use signed integers here.

So, please feel free to go ahead an propose a fix. Thanks!

[wirthjohannes]

This memtest_parallel was always a bit prototypical and has several issues/problems (and I maybe shouldn't have added it to TaPaSCo in the first place). @BabarZKhan are you using this? I reworked most of this memtest stuff (the PE and the user program). As soon as it's finished I will replace this stuff in TaPaSCo (though I will probably remove the memtest_parallel as this would will be more complicated to get right).

So I think it's not really necessary to fix this right now (as it's "only" a code smell) because it will get replaced (or removed) in the next weeks

[BabarZKhan]

@jojowi : Yes, I agree.

I am using a modified version of it. No problem if it is removed or replaced in next weeks. So I will not fix it.