My solutions for the exercises by KITbutler · Pull Request #8 · hegner/kseta

KITbutler · 2017-09-18T14:11:51Z

Modified source code for the "modern C++" exercises (except smart_pointers) and the "debugging" exercise. The rangeLoop function added in classical_looping.cpp contains a "Python range"-style start and stop input.

hegner · 2017-09-20T11:20:59Z

debugging/src/buggy.cpp

@@ -21,14 +21,13 @@ class Student {

 Student* getStudent(){
  Student* myStudent = new Student(10801983,"Mary Wright");


given this new statement and seeing no delete anywhere this code has a memory leak.

hegner · 2017-09-20T11:22:52Z

modernsyntax/classical_looping.cpp


 void iteratorLoop(std::vector<Tool>& tools){
-  for(std::vector<Tool>::const_iterator tool = tools.begin(), end = tools.end(); tool != end; ++tool){
+  for(auto tool = tools.begin(), end = tools.end(); tool != end; ++tool){


auto tool creates an unecessary copy of each object. Did you consider references?
In addition the const_iterator is const, while auto alone does not preserve constness.

What exactly does auto tool copy? The input vector itself is already passed by reference.

How can tool be const when it is incremented? Supposedly const_iterator implies the target to be const, which is not consistent with the declaration of the function input.

What you get in hand is the object, not an iterator to it. auto tool copies every single tool you are looping over. auto& tool would prevent that.
And as you are dealing with the object and not an iterator, it can as well be const.

The iterator increment ++tool does not work with const, auto seems to favour the normal iterator structure over const_iterator.

ah. sorry. you are right. I was having a different example in my head :-)

hegner · 2017-09-20T11:26:23Z

modernsyntax/classical_looping.cpp

+void rangeLoop(std::vector<Tool>& tools, const int beginning, const int ending){
+  auto tool = tools.begin();
+  std::advance(tool,beginning);
+  for(auto end = tools.end(); std::distance(tool, end) > std::distance(tools.begin(), end) - ending; ++tool){


Here std::distance is being calculated during every loop step. Is there a more performant way?

The second distance could definitely be stored. The first one might behave differently if the loop content alters the size of tools or shifts tool. For our simple print this is of course no concern so a full pre-calculation would be more efficient.

Yap. That is completely correct.

hegner · 2017-09-20T11:28:06Z

modernsyntax/lambda.cpp

  // launch a thread to increment the counter
-  std::thread increment(incrementCounter,&counter,100000);
+
+  std::thread increment([](int* counter, const unsigned int times){for (unsigned int i=0; i<times;++i){++(*counter);}},&counter,100000);


The number of iterations one could as well try to catch by value in the [] part of the lambda.

Can that syntax catch an argument passed by std::thread or would this require a pre-initialisation of a variable to catch? At the very least a variable initialisation adds another line of code where, to my understanding, the current implementation does not sacrifice performance.

To put your point to the extreme - if you specify that number in the same line as the lambda, you don't even need it as extra function parameter of the lambda ;-) Thus having an extra variable seems to better match the lambda definition. Nevertheless the current code is correct.

Which means the "better" solution depends on the intended purpose of the count. An external variable is "future-proof" for end-user input while a deliberately hardcoded value is easier to track within the function definition. Passing a fixed value as an argument as in the current version would only be desirable if it is reused multiple times within the function, correct? (then again such a function probably should not be an inline lambda to begin with)

Nothing to add to that analysis :-)

…ambda

…ounter' directly in the inline lambda

hegner · 2017-09-21T14:51:40Z

modernsyntax/lambda.cpp


-  std::thread increment([](int* counter, const unsigned int times){for (unsigned int i=0; i<times;++i){++(*counter);}},&counter,100000);
+  const unsigned int times = 100000;
+  std::thread increment([&times](int* counter){for (unsigned int i=0; i<times;++i){++(*counter);}},&counter);


would have taken it by value, but OK

hegner · 2017-09-21T14:53:52Z

debugging/src/buggy.cpp

 int main(){
  Student* myStudent = getStudent();
  printStudent(myStudent);
+  delete myStudent;


you could have used a smart pointer here.

Is there a way to do this without overwriting all standard pointer usage in the code? Initialising as a smart pointer type breaks printStudent as it expects a raw pointer as input.

This may help: http://en.cppreference.com/w/cpp/memory/unique_ptr/get

KITbutler added 8 commits September 18, 2017 11:24

iteratorLoop: changed iterator type to auto

de84d2a

exercise complete, deleted original version

e853c50

Added check-less integer range looping function and demo

a48ce46

Fixed rangeLoop to use references instead of copies

b0485d5

implemented inline lambda to replace incrementCounter

54d8715

fixed null pointer return in getStudent

74f597c

changed trailing cout to endl

a73a856

fixed printStudent to return data with the correct denomination

a79b394

hegner requested changes Sep 20, 2017

View reviewed changes

KITbutler added 4 commits September 20, 2017 16:00

simple fix for memory leak of myStudent

ca40cc0

changed unnecessary recalculation of std::distance in rangeLoop

e132108

moved 'times' counter to an external variable grabbed by the inline l…

319311d

…ambda

alternate version which removes the pointer interface and captures 'c…

fdfb4ac

…ounter' directly in the inline lambda

hegner requested changes Sep 21, 2017

View reviewed changes

		@@ -21,14 +21,13 @@ class Student {

		Student* getStudent(){
		Student* myStudent = new Student(10801983,"Mary Wright");

Conversation

KITbutler commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants