No support for building on windows without MinGW

[R-Goc]

i tried building with clang, and it mostly works. However, one of the graph test cases fails. When it gets weird though is when I try to include the library in another C file, and then i get an error for the use of an undeclared identifier arc4random, which does make sense. However, how I'm able to build the library I don't know. Is there any reason for the use of platform specific code here?

[github-actions[bot]]

Thank you for your interest in AlgoPlus, a maintainer will see your issue soon!

[R-Goc]

Quick note, tried again with w64devkit version of MinGW also doesn't work. Edit: even MSYS mingw doesn't work.

[R-Goc]

A quick change to kmeans.h that got everything compiling: kmeans(std::vector<std::vector> data, int K, int64_t MAX_ITER = 1500) : data(data), K(K) {

std::random_device rd;
std::mt19937 gen(rd());
for (int i = 0; i < K; i++) {
  int rand_num = gen() % data.size() - 1;
  this->cluster_centers.push_back(data[rand_num]);
}

You should also provide a clang format file with the library, as my clang format on save broke the whole indentation.

[spirosmaggioros]

Hi @R-Goc. Yes i agree with you that we actually never had a contributor with windows and there might be problems with building it. We once tried to build with windows 4-5 months ago and we came up with the build commands that you see on the README file.

I will look at the arc4random() error later in the day, it was a clang-tidy recommendation but it might need what you proposed as well(i made a couple of changes yesterday). If that was the only error, you can either make a PR or i will fix it later.

I guess we might need to add windows build automation as well to make sure everything works everytime, unfortunately I don't have windows, but we will see what we're going to do.

Than you for pointing this out!

[R-Goc]

Yeah, also just noticed that you're pushing ints into an array of doubles. So I'd change that as well, to actually push doubles. That would be even better. Clang tidy has some merit in what it is saying, if it was about replacing rand or random. However, there are better ways about it from what I know.

[spirosmaggioros]

These errors might exist somewhere, i wrote almost everything myself and I'm adding things everyday to check the boxes. That's why it's opensource so people can point out mistakes and fix them.

The more people getting involved, the better.

[R-Goc]

Okay, what I have now:

kmeans(std::vector<std::vector> data, int K, int64_t MAX_ITER = 1500) : data(data), K(K) {

std::random_device rd;
std::mt19937_64 gen(rd());
std::uniform_real_distribution<> distrib(
    std::numeric_limits<double>::min(), std::numeric_limits<double>::max());
for (int i = 0; i < K; i++) {
  double rand_num = distrib(gen);
  this->cluster_centers.push_back(data[rand_num]);
}

[R-Goc]

Why was there a limit of the number to the size of data? Will it break anything if there isn't one? Tests seem to pass.

[R-Goc]

I can make a fork and a PR tomorrow, also here is the test that doesn't pass on clang, but passes on mingw

REQUIRE( g.topological_sort() == v1 ) with expansion: { 0, 2, 1, 3, 4 } == { 2, 0, 1, 3, 4 }

=============================================================================== test cases: 197 | 196 passed | 1 failed assertions: 458 | 457 passed | 1 failed

[spirosmaggioros]

Yes you can make a PR and propose the changes you want.

If you use a different framework or compiler, code will act differently. For example, a graph might have multiple topological sortings, because it depends on how the queue will prioritize the node insertion, but for me when i checked the code, it was returning this particular one, so i sticked with it.

But yes, i can add some ORs on this test case.

[R-Goc]

Solved with https://github.com/CSRT-NTUA/AlgoPlus/pull/68