\$\begingroup\$I want to know if I am using the pointers correctly and if my coding style is good. those are contradicting statements. Sure you can be using pointers correctly but then your style is bad. Or you are not using pointers and your style is good. Modern C++ does not often use pointers except at the very lowest of levels.\$\endgroup\$
– Martin YorkMar 18 '17 at 20:39

\$\begingroup\$@LokiAstari I meant something more like "If pointers correct (1st priority) and then if they are, the rest of the style is good", as I read conflicting things on style.\$\endgroup\$
– ArtyerMar 18 '17 at 20:40

2

\$\begingroup\$I got what you meant. But I am also going to point out why using pointers like this is bad.\$\endgroup\$
– Martin YorkMar 18 '17 at 20:44

1 Answer
1

Comments

Most people write bad comments.
Things that should be commented are idea/decisions/why.

Writing comments that explain the code is horrible. This is because comments often fall out of date with the code, then you have to worry which is correct. Is the comment correct and the code is bad and thus I have a bug to fix, or os the code correct now I have to understand the code to re-write the comment.

That is not all these headers have. Do I need to keep the comments up to date over time?

Headers

These are headers from the "C" language.
You should note this is a completely different language.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

There are C++ equivalent for these headers.

#include <cstdio>
#include <cstdlib>
#include <cstring>

Note most C headers <HEAD.h> have a C++ equivelent <cHEAD>.

By using these headers you put all the functionality they provide in the std namespace. The headers you use put the functionality in the global namespace (though implementations may also put the functionality in std). As a result your code below where you use std::X is very likely to fail on other compilers (or at least you can not guarantee it will compile out of the box).

Design

Note: this is a good comment.

/**
* Returns the primes from 1 to <limit> as a std::size_t* that ends with a 0.
* NULL if <limit> is negative or malloc or realloc returned NULL.
*/

But the design is a bit shitty. I have to scan the prime array to find out how many I have this not very nice. Also it makes any code using it inefficient especially as that information is readily available.

Ownership semantics

The huge difference between C and C++ is ownership semantics. Note: Ownership semantics is defining who owns a resource and therefore who is responsible for releasing the resource. Only the owner can/should release the resource.

The major pain point (source of bugs) in C is caused because there is no way at the language level to define ownership semantics of a resource. In C++ we have these semantics and thus have stopped using pointers (because these have no ownership associated with them).

So a C interface that looks like this:

std::size_t* prime_sieve(std::size_t limit);

The trouble here is I don't know who owns the pointer that is returned. Is it a pointer to a static array. Is it dynamically allocated. There is no way to know from the language level based on reading that interface.

In C++ it could look like this (probably not but steering clear of vector for you).

std::unique_ptr<std::size_t[]> prime_sieve(std::size_t limit);

This basically says I (the function) am returning an array the at I dynamically allocated internally. I am handing the ownership of this array back to you and thus it is your responsibility to delete it. Note: The object std::unique_ptr will deal with deleting it.

As a result C++ does not have the issues with memory leaks that languages like C has. It also has better garbage collection (its deterministic and fine grained) than languages like C# or Java.

This is all because of ownership semantics (and the concept of RAII).

Stop using NULL

C++ has an explicit correctly types value for this: nullptr.

Unlike the macro NULL (that comes from the C language) the literal nullptr has an explicit type that does not get accidently converted to other types.

Thus it can not be used incorrectly.

Stop using C casts

C++ has its own more refined casting tools.

static_cast<>()
const_cast<>()
reinterpret_cast<>()
dynamic_cast<>()

The problem with C casts are they are exceedingly dangerous. Basically you are telling the compiler to override the type system (designed to protect you) and do as you say (even if what you say is stupid).

But to compound this C casts are exceedingly hard to spot in the code. So even a good code review may miss very dangerous casts.

The C++ casts are designed to stand out. They can be just as dangerous as a C cast (though because they perform specific subset of operations each not quite) but because they stand out they are much easier to spot while reading the code and thus review and check that you have got correct.

If I see a reinterpret_cast<>() in the code I usually go WTF and start picking through that code with a fine comb.

Stop using malloc/free/realloc

C++ has its own memory allocation functions that are built into the language.

std::size_t* primes = (std::size_t*) malloc(sizeof(std::size_t));

In C++ we would use:

std::size_t* primes = new std::size_t[1];

Actually since we want to define ownership semantics we would actually use:

std::unique_ptr<std::size_t[]> primes(new std::size_t[1]);

The two dynamic memory allocation areas (HEAP for C and FreeStore for C++) are not guaranteed to be the same region (nor is the underlying implementation of memory layout). So memory allocated can not be free'd by the other system.

So using two different systems for memory allocation becomes dangerous as not only do you need to free the memory but you need to track which system of allocation was used to allocate the memory. A simpler solution is to just not use the dynamic memory allocation system of another language.

Which brings us around to vectors.
Which does all the memory allocation in the background and you don't need to do it manually. Also as it acts just like an array it is just as easy to use as a pointer.

You have to remember to release them. The above code is correct. But basically you have to find every early return from a function and explicitly make sure that you have manually released all resources just before the return.

This is why a lot of C guidelines forbid early return (it's hard to validate that you got it all correct).

C++ introduced the concept of RAII that handles this automatically.

By using a locally scoped object the destructor of the object can release the resource when it goes out of scope (happens on function exit). So resources are automatically closed without need for the programmer to check every exit path from a function.

The standard has several types that help you do this for raw pointers. std::unique_ptr or std::shared_ptr but normally this is done as part of normal class creation. You make sure your object obeys the rule of 3/5 and then your resource are handled automatically (this is why vector is so useful all the memory management is wrapped up inside and you don't need to think about it).

This concept becomes even more important when you remember that C++ has exceptions and your function can exit because a function you called threw an exception which has cause the stack to unwind. The language guarantees that local objects destructor are called when stack is unwound thus RAII gurantess resource destruction even when exceptions are thrown.

Never do things like this. This may be easy for the compiler to read but for us poor humans this is giberish. Take some time to make it human readable.

The most expensive part of system is the programmers who read the code. Don't break them with giberish.

Prefer \n to std::end

std::cout << std::endl;

The difference is that endl also forces a flush. This is basically never needed. The automatic flushing of the streams is nearly always better. The only thing this does is potentially make the code less efficient for flushing too much.

Don't bother with return 0 in main

return 0;

If your code never returns anything but 0 then it has become convention not to add the return 0 to main. This is an indication to a reader that this application will never indicate a failure to the OS.

The dangers of unsigned

You should probably always use signed types when working with numbers (save unsigned types for bit fields).

Yes I know that the standard library uses unsigned types for std::size_t and stuff like that. But believe me the standards committee regrets that mistake and have publicly said so. BUT they are not going to change it (too late).

The problem comes from C (and thus C++) automatic integer conversions. If you pass a signed value to an unsigned parameter the compiler will silently do the conversion (even if the value is negative). The problem is you can not detect the conversion and your bounds checking will suffer:

std::size_t* prime_sieve(std::size_t limit) {
if (limit < 0) { // This will never fire.
return NULL; // As limit can never be negative.
}
// if you pass a negative value you are going to
// to do a lot of work as limit is probably a very large positive
// number in that situation.

But I can still call that function with a negative value:

prime_sieve(-1);

Will compile just as expected. Yet the test inside the function will never catch that -1.

Truly unreadable

if (!((std::istringstream) argv[1] >> limit)) {

Yes I can see what you are doing. But will everybody!

Casting a char const* to a stream (not correctly by the way). Then reading the value from the stream into limit and checking that the read worked in one statement.

Unfortunately you got it wrong. But it's still a valid statement.

What is happening here is you are taking the pointer and shifting the bits of the pointer right by limit (currently undefined). The resulting pointer is then cast to a string stream (creating a stringstream object). This is tested with the operator! and since you have not read from it will always return true.

This is a good point to advertize that you should increase your warning level well beyond normal. All warnings are logical errors in your thinking. Your compiled code should not generate any warnings.