Cubitect / cubiomes

C library that mimics the Minecraft biome generation.
MIT License
579 stars 104 forks source link

**Meaningless code** issues (multiple) #106

Closed FurryR closed 12 months ago

FurryR commented 1 year ago

I am still looking for code issues. Please do not close this issue until I reviewed the entire library.

Issue

rng.h:26

#include <stdlib.h>
// ...
#ifndef NULL
#define NULL ((void *)0)
#endif
// ...

The code try to define NULL if it does not exist, however, NULL has been already defined in <stdlib.h>. So it is meaningless.


The follow contents might be inaccurate. Do it at your own risk. By the way, I've noticed that the type of NULL is implementation-defined which means it has variant types in variant standards. For example, NULL will be an alias of predefined constant nullptr which has type nullptr_t. In this situation, you assume that the type of NULL is void* but it can also be int (any builtin integer type, i think?) or nullptr_t. It may cause conflicts or compile errors in the future development since C23 has come out. I haven't tested it in C23, just suggesting using explicit type conversions when using NULL. see cppreference.com nullptr for more information. ~I will (probably) continue tracking code issues in this repo.~

FurryR commented 1 year ago

Issue

layers.c:380

// ...
int areSimilar(int mc, int id1, int id2)
{
    if (id1 == id2) return 1;

    if (mc <= MC_1_15)
    {
        if (id1 == wooded_badlands_plateau || id1 == badlands_plateau)
            return id2 == wooded_badlands_plateau || id2 == badlands_plateau;
    }

    return getCategory(mc, id1) == getCategory(mc, id2);
}
// ...

So you are gonna test id1 == wooded_badlands_plateau || id1 == badlands_plateau, if it is true then return the same expression. However, if the expression is true, the following code will always return a true value (since there is no value modifications). So, simply replace return id1 == wooded_badlands_plateau || id1 == badlands_plateau to return 1.

Rabbit0w0 commented 1 year ago

You should open a PR instead of suggesting here.

FurryR commented 1 year ago

You should open a PR instead of suggesting here.

Sure!

Rayerdyne commented 1 year ago

About your second suggestion, that is:

replace return id1 == wooded_badlands_plateau || id1 == badlands_plateau to return 1.

It is not id1 in this line but id2, what makes more sense as it checks both ids. It would not make sense to return true whatever id2 based on a check on id1 only.

Rabbit0w0 commented 1 year ago

Also, opening such issue is considered rude and impolite.

Cubitect commented 12 months ago

I've had systems where NULL was not defined despite including inttypes.h, stdint.h and stdlib.h, so I had just defined it myself.

However, I agree that defining it to ((void*)0) is probably the least portable solution. I've changed it to now to include stddef.h instead. If the issue arises again, I'll change it to 0.

Regarding the biome check, it's id1 for the check and id2 at the return, just as Rayerdyne pointed out.