Don't use extern "C" on std libraries. Let the cpp compiler optimize itself.

NULL -> nullptr. Also you mean to assert it not assign it here.

At least for me, I am not familiar with C++. If we want to write in C, is there any good way to do it?

I would say that writing in C++, especially for these tests it is more advantageous because:

• you get a clear trace of how/what fails
• you decouple potential C memory issues, e.g. not pre-allocating pointers from tests result errors

In these case I only have 2 simple suggestions:

• Use std::array and continue to use the C interface as you would.
  The reason for this is that you allow in the future to use a wider range of built-in tools like range-for loops similar to python, that make the code much easier to read and detangle.
• Check with the IDE's suggestion.

The NULL -> nullptr is suggested by CLion and that is because the NULL definition in C is different then C++'s nullptr and C++ uses the latter for enforcing that a pointer is truely null and not a pointer assigned to (void*)0. Think of the issues of comparing addresses of empty strings (&"" == &""). That is not guaranteed to give true and depends on the compiler's optimization.

Other than that there is a genuine error here in that it is supposed to be ASSERT_EQ(symmetry == nullptr) and not an assignment symmetry = nullptr.

I see your point. But I want to confine this PR about the algorithmic side for the layer-group implementation for now. After we adopt C11 in develop branch, the corresponding refactoring would be awesome.

I'm always paranoid with C code. Fo you need to explicitly deallocate these? I prefer to use std::vector to be safe and pass c_pointer to C functions and use reinterpret_cast when needed

Do you comment in general? Or some specific point?

Both. In this case with fixed sized arrays like that, you don't need to use de-allocator?

And in general I am paranoid. E.g. if there is thread_local/static pointer and if for example someone uses the python api and the pointer is replaced without proper de-allocation, then that's a sure way to get memory leaks. If we impose and fix the issues specified by the compiler warnings in -Wall, clang-tidy inspection, and use the necessary sanitizations, then I would be less paranoid about such hypothetical issues.

I see your point. It is uneasy to take a balance of feature developments and fixing holes. I want to merge this PR without going far from the original aim of PR.

I understand, but at the very least new code should not introduce new holes. Whenever you are touching a part of a code that your IDE/clang-tidy and any other tools tells you to fix, then do a quick fix for that, otherwise the holes are never addressed.

See for example the warnings already displayed by github in the Files changed tab. (Amazed that github is able to do that 😮)

Can invalid use of pointers to arrays with different qualifiers in ISO C before C2X [-Wpedantic] and pointers to arrays with different qualifiers are incompatible in ISO C [-Wpedantic]
be postponed to fix? Or just removing const can be a solution?

I did a bit of experimentation and research and tl;dr: we don't need to address "invalid * with different qualifiers".
There aren't concerning warnings in this PR now and I did a clang-tidy test

$ pre-commit run --hook-stage manual clang-tidy --from-ref origin/develop --to-ref lan496/layer-group-refactor
clang-tidy...............................................................Passed

All seem well for this PR at least on this front. There are still the conversations that I have not marked as resolved which should be addressed and I'm all 👍 for now.

src/refinement.c:1451:9: warning: 'mat_multiply_matrix_di3' accessing 72 bytes in a region of size 24
src/spacegroup.c:1150:51: warning: 'j' may be used uninitialized

const double matrix[2][3]

#include <stdio.h>

void test_const_matrix(const int matrix[][2]){
    int vector[2] = {2,3};
//    matrix[0][0] = 2;
    matrix = vector;
    matrix = &vector;
}
void test_const_vector(const int vector[2]){
//    vector[0] = 2;
}
void test_matrix(int matrix[][2]){
    int vector[2] = {2,3};
    matrix[0][0] = 2;
    matrix = vector;
    matrix = &vector;
}
void test_vector(int vector[2]){
    vector[0] = 2;
}

int main() {
    int t_matrix[2][2] = {0};
    int t_vector[2] = {0};
    test_const_matrix(t_matrix);
    test_const_vector(t_vector);
    test_matrix(t_matrix);
    test_vector(t_vector);
    printf("Hello, World!\n");
    return 0;
}

I'll leave this conversation unresolved for now, just for visibility.

Thanks @LecrisUT.

For warning: 'mat_multiply_matrix_di3' accessing 72 bytes in a region of size 24, I have really no idea.

For warning: 'j' may be used uninitialized, this is OK, which is confirmed by assert.

For warning: 'j' may be used uninitialized, this is OK, which is confirmed by assert.

Not really, I will explain in a PR where I address the clang-tidy issues.