#587 ReferenceElements should use unsigned for indices, sizes etc.

Atgeirr Flø Rasmussen (atgeirr@sintef.no) commented on the issue:

I think it is unfortunate to use unsigned types everywhere. Even if the quantities in question are always nonnegative, such as codimensions, we may still want to take differences of them or count backwards to zero etc. I suggest just using int all over the place. Of course, this may be a controversial issue, and I do not want to start a flame war...

Other advantages of int include readability and brevity (at least compared to std::size_t).

Atgeirr

The original interface also used signed int - thats why the GenericRE do the same.

I would consider the old interface a bug as well.

There is a reason why it was implemented this way, but I don't see a reason why we should not use size_t now?!

While using int is of course possible for the Generic Reference Element, I still think using std::size_t is more natural as the index is non-negative. It takes some time, but one gets used to using it. (BTW counting backeards is not a problem.)

In other places using std::size_t has the advantage that at least on some 64bit platforms one can reference all the elements of an array that can be allocated. By not using size_t we cannot compute as large problems as the hardware would allow us.

if there is no objection I will do this next week

I'll replace the (unsigned) int's in the following files with std::size_t before the next release:

dune/geometry//referenceelements.hh dune/geometry/genericgeometry/subtopologies.hh dune/geometry/genericgeometry/referencedomain.hh dune/geometry/genericgeometry/conversion.hh dune/geometry/genericgeometry/hybridmapping.hh dune/geometry/genericgeometry/mappingprovider.hh

Matthias Wohlmuth (matthias.wohlmuth@ma.tum.de) commented on the issue:

May I ask about the situation in dune/common/geometrytype.hh: It is stated that both constructors with types unsigned int and int for the dimension are needed to distinguish between topologyId and dim. I'm not sure if I like that, but at least it seems to be intended. However, is there a reason that the member _dim is stored as unsigned char. This results in -Wtype-limits warnings when _dim is set in the int/unsigned int constructors...

Probably, that's not a big deal, but I'm just curious....

I had a closer look in the code and at some places unsigned int is used. If nobody object, I'll change the ints in unsigned ints for dim and codim. std::size_t is used for counting and would be the natural choice for the indices i and ii. Maybe I am currently a bit contemplative...

The use of int vs. unsigned int for dimensions (including mydimension, dimensionworld, and codimension) is actually annoying. However, I do not think this issue is resolved by changing these types in one class in either direction. It is rather a fundamental decision that is required here that should be made on a developer meeting. Moreover, such changes should not be made shortly before a release. It is hard to anticipate all potential problems with such a change (e.g., do you really know what will happen if you cast int( -1 ) into a std::size_t on different compilers, different architectures, etc.?).

With respect to the natural choice for indices, I disagree. std::size_t is usually an 8 byte integer on 64 bit architectures and becomes a real waste of memory for storing small index sets (like the DUNE's subentity numbering). Moreover, the standard does to my knowledge not specify std::size_t as the type to store size information in. Maybe we should add another template argument to the reference element ;-). What I actually mean to say is that in my opinion such basic decisions should long have been made on a developer meeting and that they require thorough knowledge of the C++ standard.

A similar argument holds with respect to the GeometryType. Personally, I never liked this object at all. One reason is that nearly all uses of this object can be replaced by e.g., a pointer to a singleton reference element, which actually holds all the necessary information. But let's not mourn, I think this issue, too, needs discussion.

Finally, I would like to thank you for all the effort you put into DUNE maintenance lately. It is great to someone actively trying to improve the code (and not just talking about it). Nevertheless, you should keep the bigger picture in mind. For example, it is not really important whether the reference element returns an int, an unsigned int or a std::size_t. What is important is, e.g., the return type the user expects. To this end, a lot of internal consistency is required and it is exactly this consistency that requires meetings to talk them over.

Please don't get me wrong, I am not trying to slow you down. On the contrary, I would not like to see your resources wasted on unnecessary changes that definitely require revision (says he who tends to change such inconsistencies whereever he finds them). But I do think that we both have to learn to be patient. Making such changes a real success requires a lot of discipline from all developers. And there are a lot of small construction sites within DUNE.

If you still feel like changing these types, just go ahead. All I ask is that you do it thoroughly and consistently. We have had enough easy and ill-done changes since the last release (remember the removal of compactify?).

@Matthias: _dim is an unsigned char for historic reasons. When I originally wrote the class it had two members basicType and dim. Both are basically positive integers, and will never takes very large values. Therefore I make _dim a char (and modified basicType) such that the entire class would fit into a 32bit word. I never measured whether that actually did speed up anything. Later people added more members, and the original idea got lost. Feel free to change _dim to a bigger unsigned type.

@Christoph: Go ahead. I think it is a good idea and chances of breaking anything are extremely low.

Martin, I wrote my comment while you wrote yours. Still I think that the change is not dangerous. All numbers handed in and out of the ReferenceElement class really are positive. So changing the interface doesn't change anything. Of course it should be checked whether cases such as your -1 cast happens internally. But that is easy, because the code is fairly small.

I agree with Martin. This change is way to delicate to rush it.

Actually I already replaced a large number of ints by std::size_t and it broke some template code. I'd like to see this discussed at the next developer meeting. I defer this task to the next release. I think #1092 (closed) makes more sense for this release.

Okay, no need to rush it if there are objections.

With the introduction of the Entity<0>::subEntities, this issue starts biting again. The new method returns an unsigned int while the old method Entity<0>::count returned a signed one. This leads to a lot of signed/unsigned warnings recently.

Maybe we can reach a decision for this type in the future, so that we don't have to change signedness back next year. I think the current list of possible types is: int, unsigned int, std::size_t. Any other candidates?

mentioned in issue #1140

mentioned in issue #1660

mentioned in issue core/dune-grid#10 (closed)

Let's do this. I am in favor of unsigned int but would welcome any decision for an unsigned type. Are there arguments or only feelings? Should I start some kind of vote?

mentioned in merge request core/dune-grid!53 (closed)

Let's do a manual vote on this.

What type should be used for indices, sizes etc. in ReferenceElements and related classes in dune-geometry and dune-grid, maybe in other modules, too.

Please indicate which type you like, you can choose several one and edit your answer afterwards.

a) int
b) unsigned int
c) std::size_t
d) self-defined typedef
e) need more discussion
f) abstain

I vote vor b

I vote for d into a.

If you really, really need the size of std::size_t, then there is no way around it. If not, keep it signed or use std::ptrdiff_t. I am not a fan of mixing signed and unsigned arithmetic and use unsigned types only for bit manipulation and if I really need its modulo arithmetic.

c) std::size_t.

edit: I was already wondering when I saw the notification on this bug if I was eligible for a vote at all. Now I saw the email and wonder even more. Kindly ignore this vote if I'm ineligible.

d), with the restriction that it should be an unsigned integral type (I think that was implied, but I want to make it explicit).

Always unsigned int may not be large enough in some cases, and a waste of space in other cases. Always std::size_t may a waste of space in even more cases, and is often not ideal for the architecture (int/unsigned int should usually be fastest).

d) but needs to be some unsigned integral type

I vote for d) with an unsigned type. I would prefer std::size_t, but as @joe already suggested, being able to change it to unsigned int might be favorable for problems with moderate number of indices.

I am also for d), and specify that it should be unsigned. Furthermore, the type should be exported as size_type, in analogy to what the standard library does.

I thought we already voted on the signed / unsigned question and the answer is: unsigned.

As for a typedef: Yes, we want one (called Size, because we decided that in the namespace Dune everything is CamelCase). For now, I would define it to int (to avoid a ton of warnings). Then, we could set a deadline to switch it to std::size_t (or whatever you prefer). Developers should change all uses to the newly defined type before that date.

Internally (i.e., in the implementation), I think we should go for std::uint32_t to save some memory.

Martin, you are right, we had a decision on the 2012 Developer meeting in Münster. The protocol states, we decided on "unsigned size_type". I don't know what this means and there seems to be no consensus on moving on. If we stick to unsigned size_type and implement that, I am fine with the old decision.

@gruenich That (or maybe just my interpretation) is odd in a couple of ways. size_type is not a global type but only a typedef in a few classes (e.g. std::string::size_type); there is no std::size_type. Maybe size_t was meant here?

Moreover, according to http://stackoverflow.com/a/1089204 size_t is guaranteed to be unsigned.

That said, the name size_type for a typedef would be more in line with existing classes than Size.

I also vote for d). In view of our naming convention Martin is right and it should be ::Size. Since the "canonical" name would be size_type I propose to use both. That's consistent with our decision for container-like types where we explicitly want both: ConstIterator and const_iterator.

I don't care that much for the precise type and support Martins view here.

According to our rules, we wait 14 days for comments and votes. Whoever wants to vote on the type of size()et al, please do so.

I am on the same page as @martin.nolte. But I still hate that this will cause a lot of work for downstream modules.

I'm fine with Martin's/Carsen's suggestion and agree with Markus as well: so add Size and size_type and both are uint16_t (or uint_fast16_t)

I somehow missed this, but I agree with Elias: Is size_type a good idea? If we follow STL conventions, it should be Dune::size_t (a _type suffix is only used inside of other types, not for global type aliases). That said, I'm all in favor of Size and size_t.

If we define a new type I'm in favor of Dune::Size.

Regarding the "we freely define the type" point... when we experimented with auto-vectorization, we observed that type-conversion was something that made it harder for the compiler to do autovectorization on std::array like classes. The thing is that the underlying container expects std::size_t as the natural index type and we were using a different type there. I freely defined the size type in my interface to unsigned. When I now used (as we sometime do by accident, or because different components have different integer types) use some other type, e.g. again std::size_t, the compiler has to perform two type conversions, which somehow make compiler fail to vectorize.

This said I'm in favor of something that maps to std::size_t. If we use std::size_t always, the questions would be why to use a new typedef at all.

Thus my priority list is as follows:

std::size_t
self-defined type Dune::Size + Dune::size_t, both mapping to std::size_t. If necessary we can introduce a transition period where we use some other typedef to reduce warning.

There seems to be a misunderstanding, here. I am in favor of a typedef, but on the container class (i.e., ReferenceElement). I also interpreted the previous comments in this way. Standard containers export their size as size_type, not size_t. So, if we want a dual type definition, we should go for Size and size_type.

I also do not think that a global typedef Dune::Size (or whatever) makes any sense. The perfect type for sizes is already exported by the standard library: std::size_t.

@martin.nolte: ah, ok! In that case, size_type is of course the exactly right thing to do...

I recently stumbled upon this again. It's annoying that you cannot write clean loops in user code like this

for(std::size_t i=0; i<referenceElement.size(se,seCodim.codim); ++i)
   ...

without a warning, because we still handout signed size values.

About 6 years ago it was noted that changing this should not be rushed. Maybe it's time to take action now. While I think that strong types would be nice in some places (especially for the reference elements) I'd now opt for using std::size_t whenever we know, that the quantity is conceptually unsigned (size, index, dimension, ...).

I've taken to writing

for(auto i : Dune::range(referenceElement.size(se,seCodim.codim)))
   ...

in these situations. It does not resolve all signed/unsigned problems, but at least it makes it much less annoying to make the type of the loop variable match the type of the loop bound.

This only hide's the problem because it's syntactic candy for the raw int loop.

There's little wrong with a raw int loop, as long as that is the type of both the loop variable and the loop bound.

But I'll agree that sometimes this construct only defers the signed/unsigned warning from the loop condition to some comparison in the loop body.

Carsten, please move ahead! std::size_t is probably the best choice for Dune.
I hope you are not hit by the template problems I got stuck six years ago.

Duplicate of core/dune-grid#10 (closed), which got fixed by core/dune-grid!474 (merged).

closed

changed the description

Property	Value
Reported by	Jö Fahlke (jorrit@jorrit.de)
Reported at	Aug 20, 2009 13:49
Type	Bug Report
Version	Git (pre2.4) [autotools]
Operating System	Unspecified / All
Last edited by	Christoph Grüninger (gruenich@iws.uni-stuttgart.de)
Last edited at	Nov 20, 2013 07:31

#587 ReferenceElements should use unsigned for indices, sizes etc.

Metadata

Description

Designs

Child items ...

Activity

Atgeirr Flø Rasmussen (atgeirr@sintef.no) commented on the issue:

Matthias Wohlmuth (matthias.wohlmuth@ma.tum.de) commented on the issue: