On Mon, Jan 30, 2017 at 12:05 AM, Alistair Leslie-Hughes leslie_alistair@hotmail.com wrote:
Signed-off-by: Alistair Leslie-Hughes leslie_alistair@hotmail.com
dlls/dpnet/tests/address.c | 28 +------- dlls/dpnet/tests/client.c | 150 +-------------------------------------- dlls/dpnet/tests/dpnet_private.h | 32 +++++++++ dlls/dpnet/tests/server.c | 16 ++--- ...
I have never seen a "*_private.h" header in any of the other tests, though there are a small number of "*_test.h" headers. This instance might be _especially_ confusing since it would result in both a "./dlls/dpnet/dpnet_private.h" and a "./dlls/dpnet/tests/dpnet_private.h". The second patch looks reasonable to me, though you may wish for others to weigh in.
Best, Erich
Hi Erich,
On 07/02/17 15:39, Erich E. Hoover wrote:
On Mon, Jan 30, 2017 at 12:05 AM, Alistair Leslie-Hughes leslie_alistair@hotmail.com wrote:
Signed-off-by: Alistair Leslie-Hughes leslie_alistair@hotmail.com
dlls/dpnet/tests/address.c | 28 +------- dlls/dpnet/tests/client.c | 150 +-------------------------------------- dlls/dpnet/tests/dpnet_private.h | 32 +++++++++ dlls/dpnet/tests/server.c | 16 ++--- ...
I have never seen a "*_private.h" header in any of the other tests, though there are a small number of "*_test.h" headers. This instance might be _especially_ confusing since it would result in both a "./dlls/dpnet/dpnet_private.h" and a "./dlls/dpnet/tests/dpnet_private.h". The second patch looks reasonable to me, though you may wish for others to weigh in.
Thanks for the review. I'm happy to rename the header if thats all is needed to get it committed. I'll assume dpnet_test.h is fine unless someone else states something different.
Best Regards Alistair
On Tue, Feb 7, 2017 at 12:53 AM, Alistair Leslie-Hughes leslie_alistair@hotmail.com wrote:
... Thanks for the review. I'm happy to rename the header if thats all is needed to get it committed. I'll assume dpnet_test.h is fine unless someone else states something different.
No problem! That was the thing that really stood out to me, the consolidation you're doing looks like a really solid move (personally, I hate duplicated code). The only other thing that I noticed is that is_process_elevated(), is_firewall_enabled(), and set_firewall() are all shared with the rpcrt4 tests, so it's _possible_ for there to be a lot more consolidation. However, to my knowledge, we don't have a mechanism for sharing routines like that between multiple per-dll tests. If someone is aware of such a mechanism then that may be a way to make this even better.
Best, Erich
-
Alistair Leslie-Hughes -
Erich E. Hoover