#273 [Patch] Unit test for nmg_copy

[Zhao Anqing]

Hi, Daniel

I submit another patch about the unit test of nmg_copy. The basic process of it is as following:

• Build some model as test case.
• Call nmg_clone_model(...) to get a copy.
• Check the consistency of two model (original model and the copy).

I try my best to demonstrate my understanding of the HACKING rules learned in the previous patch. Hope it's satisfying.

What's more, I write some compare functions for each level of nmg struct. According to the suggestion by Sean, I will move them into an independent file such as nmg_compare.c and perfect it (now, it's not complete) then.

As for the test result, you can see the 'testcase 3' fail because the 'nmg_clone_model' changes the order of regions in new model's r_hd. Append each region in the head of the list, so the order is reversed.(I guess it's for the performance.) It seems no problem in logic but I am not quite sure.

Thank you!

Zhao Anqing

[Daniel Roßberg]

Zhao,

Your patch is almost perfect. By reading through it I couldn't find any issue. Then I tried to compile it and got an error:

/home/rossberg/Devel/BRL-CAD/brlcad/src/librt/tests/test_nmg_copy.c: In function ‘testcase3’:
/home/rossberg/Devel/BRL-CAD/brlcad/src/librt/tests/test_nmg_copy.c:442:23: error: variable ‘r1’ set but not used [-Werror=unused-but-set-variable]
struct nmgregion *r1;

This are the kind of problems you only find with the GNU compiler. Unfortunately this is the one most users are using. That's why you should test every non-trivial change with it.

BTW, removing r1 there and ignoring the return of nmg_mrsv(m1) will fix the error.

While you're at it: Is it possible to change "test-case-x" to something more meaningful?

Regarding test case 3: The copy function shouldn't change the order of regions. Even if it isn't a change in the logic nmg_clone_model() should be fixed to produce real 1-to-1 copies. (Unfortunately I wrote this function ;)

Daniel

[Zhao Anqing]

Daniel, Thanks for the review. I made some change according to your suggestions.

• remove the unused variable 'r1'.
• change testcase's name to a more meaningful one.

Is that true I should write another patch if I want to correct the copy function? I remember your instruction that each patch should just do one thing.

Thank you!

Zhao Anqing