COIN-OR::LEMON - Graph Library

Opened 9 years ago

Last modified 8 years ago

#123 assigned enhancement

dim2::Point default constructor

Reported by: kpeter Owned by: kpeter
Priority: minor Milestone:
Component: core Version: hg main
Keywords: Cc:
Revision id:

Description

I think the default constructor of dim2::Point should set the point (0,0). It is a natural request for any class that provides operator+ to be able to sum it correctly with a template function.

I also made benchmark tests. They showed that this variant is not slower than the current one.

Attachments (1)

dim2_point_03b3e59f0b4f.patch (586 bytes) - added by kpeter 9 years ago.

Download all attachments as: .zip

Change History (15)

Changed 9 years ago by kpeter

comment:1 Changed 9 years ago by kpeter

[03b3e59f0b4f] contains this modification.

comment:2 in reply to: ↑ description ; follow-up: Changed 9 years ago by alpar

  • Priority changed from major to minor
  • Type changed from defect to enhancement
  • Version set to hg main

Replying to kpeter:

I think the default constructor of dim2::Point should set the point (0,0). It is a natural request for any class that provides operator+ to be able to sum it correctly with a template function.

I think it is far from being a natural request.

I also made benchmark tests. They showed that this variant is not slower than the current one.

Which kind of benchmark did you made?

Alpar

comment:3 in reply to: ↑ description ; follow-up: Changed 9 years ago by alpar

Probably it is better to raise a topic like this on the mailing list first.

comment:4 in reply to: ↑ 2 ; follow-up: Changed 9 years ago by kpeter

  • Status changed from new to assigned

Replying to alpar:

I think it is far from being a natural request.

We have an argument about this question months ago. Like it or not it is an important request in C++. Please try to write a template function that can sum int, double etc. values as well as dim2::Point<T> values correctly without this modification.

If we can't write such a function, then it is proved that the suggested default constructor is a natural request.

Replying to alpar:

Which kind of benchmark did you made?

E.g. I created very large std::vector<dim2::Point<T> > structures using int, double etc. as T.

comment:5 in reply to: ↑ 3 Changed 9 years ago by kpeter

Replying to alpar:

Probably it is better to raise a topic like this on the mailing list first.

Why?

Small topics like that are usually neglected and then forgotten on the list. I think this ticket system is for tickets like that (among others). A ticket can be closed with wontfix status, if it is not reasonable.

comment:6 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by alpar

Replying to kpeter:

Replying to alpar:

I think it is far from being a natural request.

We have an argument about this question months ago. Like it or not it is an important request in C++. Please try to write a template function that can sum int, double etc. values as well as dim2::Point<T> values correctly without this modification.

Well, let me try. What about this?

template<class T, class I>
T sum(I begin, I end, const T &init)
{
  T s(init);
  for(I i=begin;i!=end;++i) s+=*i;
  return s;
}

This is probably the only correct approach.

If we can't write such a function, then it is proved that the suggested default constructor is a natural request.

As you can see above, we can. What then?

comment:7 in reply to: ↑ 6 Changed 9 years ago by alpar

  • Milestone changed from LEMON 1.0 release to LEMON 1.1 release

So, I'm not convinced at all neither to have a default constructor nor to keep the present setup.

However, it isn't break the API if we change it later, thus we can safely postpone this ticket after the 1.0 release.

comment:8 follow-up: Changed 9 years ago by kpeter

Replying to alpar:

Well, let me try. What about this?

You are right. I should have specified the problem more exactly: I'm looking for a template sum function without getting an explicit null-value for T, since I think your approach is uncomfortable in many cases.

See e.g. the maps_summary.cc demo file in the svn repo. As far as I know the suggested standard way of summing in C++ is the one that is used in the summary() function. It not works for dim2::Point.

In fact, it is not clear enough whether this change is necessary or not, but it is clear that it have advantages, and it doesn't have any disadvantage. Am I right? If it is true, than I think it should be accepted.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 9 years ago by alpar

Replying to kpeter:

Replying to alpar:

Well, let me try. What about this?

You are right. I should have specified the problem more exactly: I'm looking for a template sum function without getting an explicit null-value for T, since I think your approach is uncomfortable in many cases.

I understand, but typically you do not want to do it in this form if you want to have a uniform interface of the similar functions (i.e. creating an aggregate value of a container using an aggregating operator.) Think of e.g. max and min.

See e.g. the maps_summary.cc demo file in the svn repo. As far as I know the suggested standard way of summing in C++ is the one that is used in the summary() function.

Hopefully not.

In fact, it is not clear enough whether this change is necessary or not, but it is clear that it have advantages, and it doesn't have any disadvantage. Am I right?

If the second statement is true, then yes, you are right.

If it is true, than I think it should be accepted.

Nobody said the opposite (not me, at least). I just say that we have much more important things to do now. That is why it was postponed until the next release.

comment:10 in reply to: ↑ 9 Changed 9 years ago by alpar

  • Milestone changed from LEMON 1.1 release to LEMON 1.2 release

Replying to alpar:

Nobody said the opposite (not me, at least). I just say that we have much more important things to do now. That is why it was postponed until the next release.

It seems to be still true.

comment:11 follow-up: Changed 8 years ago by kpeter

Could you tell me any disadvantage of this modification?

comment:12 in reply to: ↑ 11 Changed 8 years ago by alpar

Replying to kpeter:

Could you tell me any disadvantage of this modification?

My concerns are the followings.

  • Conceptually bad. The purpose of a constructor should be to initialize the data structure and not to set it to some comfortable default value, especially for tiny structures like this. Though, I admit this misuse is quite frequent in C++.
  • Potentially hides bugs. If a datastructure is used uninitialized, then the chance is high that the compiler will warn about it. But this patch hides this kind of bugs.
  • Restricts the usage, as it requires the existence of an int->X conversion, where X is the underlying data type.
  • May degrade performance due to double initialization.

Neither of these problem are serious. But the only thing I can see in the other pan of the balance is that it makes it possible to implement a sum() function with an improper interface. Quite weak.

comment:13 Changed 8 years ago by kpeter

I think, only "Restricts the usage" could be a somewhat relevant reason of yours. But it is "quite weak".

I think, this discussion is not about performance or usability any more. It is rather about your personal antipathy against a common concept/guideline in C++. What you declared in the first point ("Conceptually bad") is not objective, it is a matter of taste.

In fact, I don't find this conceptual question so important, so I suggest closing this ticket with wontfix status.

comment:14 Changed 8 years ago by kpeter

  • Milestone LEMON 1.2 release deleted
Note: See TracTickets for help on using tickets.