Some constructive criticism & a few pointers to help improve the code:
First, try not to use std::cout from a function outside of main() unless it is specifically intended for I/O. I know this doesn't seem to matter much to people these days but it's a design concept that is much preferred, especially for professional-level code. Another note, for the same design reasoning, is that you should always try to 'return' from your code, even if it's void. A simple 'return;' statement is sufficient and lets various code-scanning tools(and documentation tools) better handle the function.
Second, your flow logic could be improved. Since both the true and false forks of your conditional check result in a 'loc' increment, you should keep it outside the condition. Even better, since the true fork does nothing but the increment, you can reverse the logic and eliminate it altogether.
while (loc < rvPrimes.size()) {
if (rvPrimes[loc] != 0) { // OR if (rvPrimes[loc]) since it's a boolean check against value
pVal = rvPrimes[loc];
for (int i = 1; (loc + (i * pVal)) < rvPrimes.size(); ++i)
rvPrimes[loc + (i * pVal)] = 0;
}
++loc;
}
Third, since 'loc' and 'i' are both used for indexes, their value should never be negative. Thus, you would be best to use 'unsigned int' for these variables. Further, note that std::vector<T>.size() returns an unsigned value(size_type is unsigned) and thus (int + (int * int)) should return an int and int < unsigned int should technically throw a compiler warning. The reason for this is that it is possible to have a vector that has more than 2,147,483,647 members(though not necessarily wise). If this is the case, then (loc + (i * pVal)) could potentially ALWAYS be < rvPrimes.size() and you're stuck with an infinite loop.
Fourth, your inner iterator could potentially be implemented as a register allowing for a tighter loop. The compiler may do this automatically but not always. Don't abuse the register keyword, as there are only a few registers available at one time, but rather use it sparingly where you can potentially reap a large benefit. A single inner loop that runs many times over each element of a list that can measure a million or more in size is a very good reason to use a register.
Finally, rather than leaving values in the vector as 0, it might be best to remove the entry completely to keep a cleaner and smaller vector for speed and memory efficiency. This may not be important to this function specifically, or even this test-case, but if you're planning to use this function in other code where you want a list of primes to pass around, it might be best to use as little memory as possible. Further, if you're appending to the vector in a separate thread(using intelligent queuing with proper lock handling; vectors by themselves are not necessarily thread-safe!!) then its size can obviously grow quite large rather quickly.
An example including some of the aforementioned suggestions:
#include <iostream>
#include <vector>
// shortcut macro for uncluttered calculation
#define C(l, i, p) (l + (i * p))
void primeListing(std::vector<int> &rvPrimes) {
unsigned int loc = 0, sz = rvPrimes.size();
int pVal; // could pVal legitimately be negative???
// shortcut macro using local variables for uncluttered calculation
#define D(i) C(loc, i, pVal)
while (loc < sz) {
if (rvPrimes[loc]) { // true if rvPrimes[loc] has non-zero value
pVal = rvPrimes[loc];
for (register unsigned int i = 1; D(i) < sz; ++i)
rvPrimes[D(i)] = 0;
}
++loc;
}
// optional; eliminates 0 values, leaving a clean list of only primes
for (
std::vector<int>::iterator it = rvPrimes.begin();
it != rvPrimes.end();
++it
) // I know this looks weird but some still use 80-column terminals
if (!*it) rvPrimes.erase(it--); // post-decrement
return;
}
int main(void) {
std::vector<int> primes;
for (unsigned int i = 2; i < 1000; ++i) primes.push_back(i);
primeList(primes);
for (
std::vector<int>::iterator it = primes.begin();
it != primes.end();
++it
) // again, looks weird but it's easy to read and understand in 80 cols
std::cout << *it << std::endl;
return 0;
}
Overall, this is a good exercise and a worth-while pursuit. I would recommend continuing to work on improving this code to see how efficient & elegant you can make it. If you have questions, just voice them and I'm sure someone will assist.
