Usage of unsigned and <stdint.h> types.

[froj]

Let's discuss this (see last comment on issue #3).

[antoinealb]

My opinion on the topic is that we should only use them when they are really required. Most of the time we just want to pass around a smallish number and this is just visual noise. But I agree that they will be required in some places.

[pierluca]

I don't think it changes much in terms of visual noise if we're using this systematically rather than sporadically. It's unambiguous: unsigned or not, size, period. The only advantage is that it prevents potential cross-platforms bugs.

It's just a matter of deciding on a common policy, really. Who's in favor ? who's against?

On Thu, Jun 12, 2014 at 11:48 PM, Antoine Albertelli < notifications@github.com> wrote:

My opinion on the topic is that we should only use them when they are really required. Most of the time we just want to pass around a smallish number and this is just visual noise. But I agree that they will be required in some places.

— Reply to this email directly or view it on GitHub https://github.com/cvra/platform-abstraction/issues/7#issuecomment-45952492 .

[antoinealb]

I am against using it everywhere.

[froj]

I agree that something like semaphore->count should be at least unsigned, but for something like

int i;
for (i = 0; i < CONST_SMALLER_THAN_256; i++) {
   ...
}

I think it would be wrong to declare uint8_t i;.

[31415us]

I like types and would like to see them used. @froj: semaphore->count is actually a case where you absolutely want a signed type (=

[antoinealb]

@31415us Used everywhere ? Like even in the case @froj described ?

+1 for semaphore->count being signed, you don't want it to go skyrocket if you go under zero :D

[froj]

Well, logically it is a unsigned value and there should be atomic tests for zero whenever it's decremented, no?

[antoinealb]

keyword is should

[31415us]

@antoinealb: yeah even in that case although you could argue to use a standard int32_t in such cases I don't see why it would be 'wrong'

@froj : normally semaphores are absolutely allowed to have a negative count value (i'll recheck that in my course resource .pdf that I sent you)

[antoinealb]

Well using a standard int32_t is not an issue unless you plan to deploy it on < 32 bits machine. By the way do you plan to use this abstraction stuff in your laser beacons ?

[froj]

you could argue to use a standard int32_t in such cases

Why int32? What you want is an integer and you don't really care about it's size. So you should take the architectures standard length of an integer, thus int.

normally semaphores are absolutely allowed to have a negative count value

Would be cool if you could explain why and what a negative count would mean.

[msplr]

Would be cool if you could explain why and what a negative count would mean.

it tells you, how many tasks are waiting for the semaphore to be released.

[31415us]

It's just that I prefer to exactly know the size of my types, it allows me to better think about overflow issues and such stuff. On the other hand I absolutely see your point and can't find a valid counter argument at the moment. @nuft thank you for answering the other question ,-)

[pierluca]

Well using a standard int32_t is not an issue unless you plan to deploy it on < 32 bits machine. Why int32? What you want is an integer and you don't really care about it's size.

I have to disagree with both these statements. Using "int" because "you don't have to choose" just means that you're allowing yourself uncertainty expecting the code won't bite back at you. That could be considered arrogant :-P

If you clarify the size upfront, you're either using something smaller than what your architecture would allow (completely okay) or using the size you need in that part of the program (thus avoiding overflows). It's a tiny overhead during the development, but it makes life easy during the debugging phase.

[antoinealb]

Personally I don't mind changing my coding habits for this, it just feels useless to me. But if you guys prefer coding in a project with that kind of types, I am ok with it.

[froj]

Would be cool if you could explain why and what a negative count would mean.

it tells you, how many tasks are waiting for the semaphore to be released.

Thanks! In uC/OS-II it's unsigned, though...

Personally I don't mind changing my coding habits for this, it just feels useless to me. But if you guys prefer coding in a project with that kind of types, I am ok with it.

Same here.

[antoinealb]

Preparing a patch for this now.

[msplr]

normally semaphores are absolutely allowed to have a negative count value There is no "right" way for how semaphores are implemented.

I have to agree, that we should specify the size, but how about making it configurable with a preprocessor define? Then we only have to decide for signed or unsigned.

In uC/OS-II it's unsigned, though...

It is probably useful to keep it the same as in the actual implementation from the os.

[antoinealb]

Making stuff configurable using #ifdef if that's what you mean makes testing harder : you have to test every combination of configuration and it makes maintenance globally more difficult.

[antoinealb]

We should also probably put this in the coding style as well.

[Stapelzeiger]

Personally I prefer to use standard int types (which are at least 16bit) in cases where we don't care about the exact size. (like for loop counters, semaphore counters, etc) But if you really want to use stdint types, we could use types like int_fast16_t instead of int. This way the compiler can still chose the fastest type with at least the specified width.

[antoinealb]

I am against int_fast_*_t. Too much to type, too little benefit.

[pierluca]

+1

On Fri, Jun 13, 2014 at 2:57 PM, Antoine Albertelli < notifications@github.com> wrote:

I am against intfast*_t. Too much to type, too little benefit.

— Reply to this email directly or view it on GitHub https://github.com/cvra/platform-abstraction/issues/7#issuecomment-46007658 .

[Stapelzeiger]

So the conclusion is to use the smallest [u]int*_t type possible for the task? (A for loop counter that loops just a few times would be uint8_t, the index in a buffer (array) uint16_t and wheel encoders int32_t)

[antoinealb]

I think we can use an int for simple case such as for loop counter, since we know it will fit at least an int16_t.

[Stapelzeiger]

I agree. If everyone is OK with this we should put the rules in the coding style.

[antoinealb]

Since this is kind of an "hot" topic, I would like to have the opinion of other team members.

[pierluca]

@Stapelzeiger

So the conclusion is to use the smallest [u]int*_t type possible for the task?

I don't think the point is to necessarily use the smallest possible in an "optimization" sense but rather removing the cross-platform ambiguity that comes from using variable-size types. There are loops where we might want to use uint16_t (on the x86, mainly) and buffers with uint8_t.

I rather consider this to be something that

1. helps debugging and removes cross-platform changes
2. forces us to think about the requirements on our code

[Stapelzeiger]

Why would you use a larger int than needed? Edit: if it's a question of optimization the int_fast*_t would be more appropriate.

There are loops where we might want to use uint16_t (on the x86, mainly)

assuming it loops only a few times

for the buffer example I was assuming the buffer is > 256 bytes, otherwise I would use an uint8_t

[pierluca]

(edit: sorry for double post)

Why would you use a larger int than needed?

I wouldn't, but when in doubt, it's not a big deal. Optimization comes later in development.

Edit: if it's a question of optimization the int_fast*_t would be more appropriate.

Exactly, to me the point here is NOT optimization, but rather removing ambiguity and cross-platform differences.

[Stapelzeiger]

I think we both agree: using the smallest int that you are sure is big enough for the task.

[antoinealb]

Fixed in PR #12 .