Ivan Gyurdiev ivg231@gmail.com writes:
Type: Cleanup
Why: The const qualifier is unnecessarily restrictive. I intend to allocate and free such data on the heap in a future patch (in addition to the current const data).
I still don't see any reason for this. Please fix your code to not require that change by simply saving the pointer you are going to free somewhere else.
Alexandre Julliard wrote:
Ivan Gyurdiev ivg231@gmail.com writes:
Type: Cleanup
Why: The const qualifier is unnecessarily restrictive. I intend to allocate and free such data on the heap in a future patch (in addition to the current const data).
I still don't see any reason for this. Please fix your code to not require that change by simply saving the pointer you are going to free somewhere else.
It's already marked const in the parameters of the set and get functions, which means it can't be modified there (arg 3): + void (*set_handler) (IDirect3DDevice9* device, const struct state_test* test, const void* data_in); + void (*get_handler) (IDirect3DDevice9* device, const struct state_test* test, const void* data_out);
I guess you get protection against modifying it in the init and teardown functions (which is never going to happen - those are two functions you write at the same time, located directly next to each other)... at the cost of significantly reducing code readability. I would have to define a "test_allocations" structure or something like that, and put the very same pointers in there, and then free them at the end...
Ivan Gyurdiev ivg231@gmail.com writes:
It's already marked const in the parameters of the set and get functions, which means it can't be modified there (arg 3):
- void (*set_handler) (IDirect3DDevice9* device, const struct
state_test* test, const void* data_in);
- void (*get_handler) (IDirect3DDevice9* device, const struct
state_test* test, const void* data_out);
Which is precisely why the data pointers have to be const too. Since the tests don't modify anything, it must be possible to give them constant data. With your scheme you need to cast const off, which is ugly and will cause compiler warnings.
Alexandre Julliard wrote:
Ivan Gyurdiev ivg231@gmail.com writes:
It's already marked const in the parameters of the set and get functions, which means it can't be modified there (arg 3):
- void (*set_handler) (IDirect3DDevice9* device, const struct
state_test* test, const void* data_in);
- void (*get_handler) (IDirect3DDevice9* device, const struct
state_test* test, const void* data_out);
Which is precisely why the data pointers have to be const too. Since the tests don't modify anything,
Well... the way I see it, the test is composed of get/set, while the init/teardown is just setup for the test, which may or may not include allocating the data pointers on the heap (and correspondingly freeing that data). Therefore the data is const in one place, and non-const in the other where I don't know what the test is going to do (by virtue of polymorphism). Yes, I suppose you could look at that as two completely separate usages of the same data, requiring separate pointers, but that seems rather odd to me.
Actually the other direction is possible too, keeping the data const, and just casting off the const in the teardown function, but that seemed uglier.
it must be possible to give them constant data. With your scheme you need to cast const off, which is ugly and will cause compiler warnings.
I don't know about ugly, but the casts actually prevent warnings, instead of causing them... Is there a compiler flag that you use to cause warnings? If so, please let me know what it is, and I'll fix the code.
Well, ok, I'm starting to see this your way now. I'll make this change, because of two reasons:
1) I'll need some kind of data structure called "test_context" anyway, whether or not those pointers are copied there. Future tests will probably require the ability to store additional data during the lifetime of the test, that's not one of those data sample points.
2) The teardown function in some of those examples frees certain data pointers, and not others. That's because the same data is used on two pointers, so you can't free it twice. However, that's ugly, and could be better handled using a separate copy of the pointer.
Ivan Gyurdiev ivg231@gmail.com writes:
I don't know about ugly, but the casts actually prevent warnings, instead of causing them... Is there a compiler flag that you use to cause warnings? If so, please let me know what it is, and I'll fix the code.
-Wcast-qual. That's what all the recent patches from Andrew Talbot are about, to make it possible to turn this on by default.
-
Alexandre Julliard -
Ivan Gyurdiev