Re: [waffle] [PATCH 16/33] third_party/threads: import c11 threads emulation wrappers
Emil, I'm finished reviewing for today, and am stopping here at patch 16. I'll resume reviewing tomorrow. I see that you're collecting reviewed-by tags and cleanups on versioned brancehs (for-upstream-3.*). I will defer cherry-picking patches off the mailing list onto master; and instead will wait for you to send merge requests, because I assume that was your intended workflow. If you want to do things differently, let me know. ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH 15/33] c99: use strerror_s over strerror_r under Windows
On 07/07/2014 10:28 AM, Emil Velikov wrote: > +/* > + * strerror_r (strictly speaking not C99) ^^ You documented what's wrong with defining strerror_r in a C99 compatibility header. But it would be nice to also document what's *right*. Maybe say "Not in C99, but in POSIX.1-2001.". Oh... I'm nit-picking. /me shuts up and takes the code This patch is Reviewed-by: Chad Versace > + */ > +#if defined(_WIN32) > +#define strerror_r(errno,buf,len) strerror_s(buf,len,errno) > +#endif ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH 14/33] c99: add snprintf and strcasecmp
On 07/07/2014 10:28 AM, Emil Velikov wrote: > +#define strcasecmp _stricmp > +#define snprintf c99_snprintf ... > +static inline int > +c99_vsnprintf(char* str, size_t size, const char* format, va_list ap) Did you intentionally omit `#define vsnprintf c99_vsnprintf`? If so, then please document why in the header. If not, then please add it to the header for consistency. ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH 13/33] c99: define inline keyword and use it across waffle core
Patches 12 and 13 are Reviewed-by: Chad Versace ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH 11/33] waffle_test: build as a static library
Makes sense to me. Patch 11/33 is Reviewed-by: Chad Versace ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH 10/33] cmake: build with fPIC when possible
On 07/15/2014 07:44 AM, Jose Fonseca wrote:
> On 07/07/14 18:28, Emil Velikov wrote:
>> Some of our third_party libraries may be build without it thus we'll
>> fail at
>> link tim.
>>
>> Signed-off-by: Emil Velikov
>> ---
>> cmake/Modules/WaffleDefineCompilerFlags.cmake | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/cmake/Modules/WaffleDefineCompilerFlags.cmake
>> b/cmake/Modules/WaffleDefineCompilerFlags.cmake
>> index 4d149c8..96a7a10 100644
>> --- a/cmake/Modules/WaffleDefineCompilerFlags.cmake
>> +++ b/cmake/Modules/WaffleDefineCompilerFlags.cmake
>> @@ -50,6 +50,8 @@ if(waffle_on_linux)
>> waffle_add_c_flag("-Werror=missing-prototypes"
>> WERROR_MISSING_PROTOTYPES)
>> endif()
>>
>> +waffle_add_c_flag("-fPIC" WITH_FPIC)
>> +
>> waffle_check_thread_local_storage()
>>
>> if(waffle_has_tls)
>>
>
> Another way of fixing this is doing like in Apitrace:
>
> -
> https://github.com/apitrace/apitrace/blob/master/cmak/ConvenienceLibrary.cmake
>
>
> -
> https://github.com/apitrace/apitrace/commit/c56b9acc6abcae465b916f6ebafa3a282d1f36fc
>
>
> This gives more control. For example, unlike a blanket -FPIC flag,
> executables won't be needlessly compiled with FPIC.
Jose, your add_convenience_library() function may be unneeded. See the
manpage quote below.
Emil, did you intend to compile *everything*, including executables,
with PIC?
On master, Waffle is already building shared libraries with PIC. I
verified it with:
$ ninja clean
$ ninja -v libwaffle-1.so | grep -e '-o libwaffle'
... cc -fPIC ... -o lib/libwaffle-1.so.0.3.90 ...
CMake's default behavior is to build shared libraries with PIC. From
man://cmake(1) :
>set_target_properties
>...
>POSITION_INDEPENDENT_CODE
> Whether to create a position-independent target
>
> The POSITION_INDEPENDENT_CODE property determines whether posi‐
> tion independent executables or shared libraries will be cre‐
> ated. This property is true by default for SHARED and MODULE
> library targets and false otherwise. This property is initial‐
> ized by the value of the variable CMAKE_POSITION_INDEPEN‐
> DENT_CODE if it is set when a target is created.
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH 08/33] core: return false on failure in waffle_window_resize
Patch 8/33 is Reviewed-by: Chad Versace ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH 09/33] wflinfo: silence signed/unsigned comparison warning
On 07/07/2014 10:28 AM, Emil Velikov wrote:
> The variable i is used as and compared vs unsigned int.
> Change it's type so silcence the compiler warning.
>
> Signed-off-by: Emil Velikov
> ---
> src/utils/wflinfo.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/utils/wflinfo.c b/src/utils/wflinfo.c
> index 3508cc6..58028f9 100644
> --- a/src/utils/wflinfo.c
> +++ b/src/utils/wflinfo.c
> @@ -778,7 +778,7 @@ gl_has_extension_GetStringi(const char *name)
> error_printf("Wflinfo", "glGetIntegerv(GL_NUM_EXTENSIONS) failed");
> }
>
> -for (int i = 0; i < num_exts; i++) {
> + for (uint32_t i = 0; i < num_exts; i++) {
> const uint8_t *ext = glGetStringi(GL_EXTENSIONS, i);
> if (!ext || glGetError()) {
> error_printf("Wflinfo", "glGetStringi(GL_EXTENSIONS) failed");
>
This patch is correct as-is, and is
Reviewed-by: Chad Versace
The patch in your for-upstream-3.1 branch (commit
010aeff75cd1129ade5a55264efa396d4e7e9f73) is incorrect, though. If you
run `sed s/GLint/GLuint/g' on commit
010aeff75cd1129ade5a55264efa396d4e7e9f73 (on the commit message too),
then the patch becomes correct. If do the sed, then that patch also has
my r-b.
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH 07/33] linux: plug a memory leak
Patch 7/33 is Reviewed-by: Chad Versace ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH 07/33] linux: plug a memory leak
On 07/07/2014 10:28 AM, Emil Velikov wrote: > Chad, do you have a plan/idea how to handle this ? I'm assuming that > your plan is to tackle this once waffle_init is gone/replaced with a > better solution (issue #7). There appears to be a straightforward solution: add a new API call waffle_finish(void) that tears down the global state set up by waffle_init(). The caveat is that if the user makes the calls in this sequence: waffle_init(platform=any_egl_platform) ... waffle_finish() ... waffle_init(platform=a_different_egl_platform) BOOM! ...then older versions of Mesa crashes inside libEGL. I think the issue existed as recent as Mesa 10.0. Despite the danger of crashing when on buggy drivers, I think adding waffle_finish() to the API is a good idea. And as you point out, deprecation of waffle_init() will lead to a different, but orthogonal solution, to memory leaks. ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] Some new and old fixes
On 15 July 2014 19:09, Jose Fonseca wrote: > On 15/07/14 18:14, Emil Velikov wrote: >> >> On 15/07/14 17:30, Jose Fonseca wrote: [snip] >> I would appreciate if you can find out where SDKDDKVer.h and windows.h are >> located for the v120 toolset and which program provided them. > > > Here it is: > >> call "C:\Program Files (x86)\Microsoft Visual Studio >> 12.0\Common7\Tools\\..\..\VC\vcvarsall.bat" x86 > >> echo INCLUDE=%INCLUDE% > INCLUDE=C:\Program Files (x86)\Microsoft Visual Studio > 12.0\VC\INCLUDE;C:\Program Files (x86)\Microsoft Visual Studio > 12.0\VC\ATLMFC\INCLUDE;C:\Program Files (x > 86)\Windows Kits\8.1\include\shared;C:\Program Files (x86)\Windows > Kits\8.1\include\um;C:\Program Files (x86)\Windows Kits\8.1\include\winrt; > >> dir "c:\Program Files (x86)\Windows Kits\8.1\Include\um\windows.h" [snip] >> dir "c:\Program Files (x86)\Windows Kits\8.1\Include\shared\sdkddkver.h" [snip] > Jose Interesting... MSVC 2013 seems to mandate that you need "Windows SDK for Windows 8.1" a note which I'm yet to see during the installation. Perhaps it's in the documentation somewhere ? Which would explain why (a month or so ago) installing "Windows SDK for Windows 7" one did not help and I went for the "Microsoft Visual Studio 2013 SDK". The latter of which did provide some files but... Things seems to be building (need to test) now though this seems rather "inspiring" - building under Win 7, but using the Win 8 SDK, for targets in range Win XP - Win 7. What a lovely story this is :) Big thanks Jose, Emil ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
