search

esa-tu-darmstadt / tapasco

The Task Parallel System Composer (TaPaSCo)
GNU Lesser General Public License v3.0
105 stars 24 forks source link

comparing integer expressions of different signedness in memory benchmarking file #273

Open BabarZKhan opened 3 years ago

BabarZKhan commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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:

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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.