[Mesa-dev] [PATCH v2 7/8] swr: Added swr windows support

[Emil Velikov]

On 9 November 2016 at 23:18, George Kyriazis <george.kyriazis at intel.com> wrote:
> - moving some header files around for proper inclusion of windows.h
> - OS agnostic loading of arch-specific loadable modules
> - PUBLIC function declaration
> - better handling on NOMINMAX around windows.h inclusion.
> ---
Hmm having a second read - seems like you've provided a broken scons
builds and here you break the autotools one.
Please do _not_ do that. Think how to manage things without forcing
everything in one big patch and without causing (intentional)
intermittent breakage.

An idea off the top of my head:
 - Patches 1-3
 - Add autotools CPPGLAGS (8/8 -> 1/8)
 - Reorder includes - windows.h & NOMINMAX fixes
 - Add new API (swr_gdi_swap), resolve duplicate definitions yet
single declaration [+ add associated PUBLIC]
 - scons build bits 4-6
 - at any point: misc cleanups (swr_get_winsys, swr_get_displaytarget other)

If anything better comes to mind, please go ahead.


>  typedef pipe_screen *(*screen_create_proc)(struct sw_winsys *winsys);
>
>  struct pipe_screen *
>  swr_create_screen(struct sw_winsys *winsys)
>  {
Having a closer look it seems that you're having two swr_create_screen
symbols. One (private) in the loader and another public in the
backends.

Rename one of the two, adding distinct declarations and keeping PUBLIC
with the function definition. At the moment you'll export
swr_create_screen from both driver and backends and dlsym(foo,
"swr_create_screen") can return the loader swr_create_screen address.


> @@ -35,13 +42,6 @@ extern "C" {
>  #include "gallivm/lp_bld_limits.h"
>  }
>
Sidenote: doing extern "C" { #include "foo" } is a _very_ bad idea
[1]. Please try to resolve this by properly annotating things as
extern C within the headers themselves.

[1] http://developers.redhat.com/blog/2016/02/29/why-cstdlib-is-more-complicated-than-you-might-think/



> -struct sw_winsys *
> -swr_get_winsys(struct pipe_screen *pipe)
> -{
> -   return ((struct swr_screen *)pipe)->winsys;
> -}
> -
> -struct sw_displaytarget *
> -swr_get_displaytarget(struct pipe_resource *resource)
> -{
> -   return ((struct swr_resource *)resource)->display_target;
> -}
These are the respective hunk in swr_public.h are independent cleanups.

Thanks
Emil