Re: [LyX/2.2.x] Fix bugs #9598 and #10650

[Guillaume MM]

so as not to lay a trap for the developers who are going to come 
after me.



"to maintain the code after me" (avoiding any unintentional double
meaning due to my uncolloquial English).

Re: [LyX/2.2.x] Fix bugs #9598 and #10650

[Guillaume MM]

Le 08/06/2017 à 26:72, Enrico Forestieri a écrit :
> On Thu, Jun 08, 2017 at 12:50:19AM +0200, Guillaume MM wrote:
>
>> Le 05/06/2017 à 23:15, Enrico Forestieri a écrit :
>>> commit 59c22bd7b604a3ba9e0e78f7c51cb601f08d0192
>>> Author: Enrico Forestieri
>>> Date:   Mon Jun 5 23:14:48 2017 +0200
>>>
>>>   Fix bugs #9598 and #10650
>>> ---
>>
>>> +// gcc < 4.8.0 and msvc < 2015 do not support C++11 thread_local
>>> +#if defined(__GNUC__) && ((__GNUC__ == 4 && __GNUC_MINOR__ < 8) || 
__cplusplus < 201103L)

>>> +#define THREAD_LOCAL_STATIC static __thread
>>> +#elif defined(_MSC_VER) && ((_MSC_VER < 1900) || __cplusplus < 
201103L)

>>> +#define THREAD_LOCAL_STATIC static __declspec(thread)
>>> +#else
>>> +#define THREAD_LOCAL_STATIC thread_local static
>>> +#endif
>>> +
>>
>>
>> According to Stephan in this discussion:
>>
>> https://www.mail-archive.com/lyx-devel@lists.lyx.org/msg196176.html
>>
>> it is unfortunately not possible to use thread_local on Mac before some
>> time, it seems. I would actually be happy to hear the contrary.
>
> Note that __thread appears to be supported since OSX 10.7, and
> clang always takes the first branch because for historical reasons it
> currently defines __GNUC__ = 4 and __GNUC_MINOR__ = 2, so it
> should work on 10.7 and above. But I agree that this is confusing
> and fragile, so I will clarify the code so as not to lay a trap
> for the developers who are going to come after me.
>

Ok, good to know, thanks.

Guillaume

Re: [LyX/2.2.x] Fix bugs #9598 and #10650

[Enrico Forestieri]

On Tue, Jun 20, 2017 at 02:45:19AM +0200, Guillaume MM wrote:

> Le 08/06/2017 à 02:07, Enrico Forestieri a écrit :
> > On Thu, Jun 08, 2017 at 12:50:19AM +0200, Guillaume MM wrote:
> > 
> > > Le 05/06/2017 à 23:15, Enrico Forestieri a écrit :
> > > > commit 59c22bd7b604a3ba9e0e78f7c51cb601f08d0192
> > > > Author: Enrico Forestieri
> > > > Date:   Mon Jun 5 23:14:48 2017 +0200
> > > > 
> > > >   Fix bugs #9598 and #10650
> > > > ---
> > > 
> > > > +// gcc < 4.8.0 and msvc < 2015 do not support C++11 thread_local
> > > > +#if defined(__GNUC__) && ((__GNUC__ == 4 && __GNUC_MINOR__ < 8) || 
> > > > __cplusplus < 201103L)
> > > > +#define THREAD_LOCAL_STATIC static __thread
> > > > +#elif defined(_MSC_VER) && ((_MSC_VER < 1900) || __cplusplus < 201103L)
> > > > +#define THREAD_LOCAL_STATIC static __declspec(thread)
> > > > +#else
> > > > +#define THREAD_LOCAL_STATIC thread_local static
> > > > +#endif
> > > > +
> > > 
> > > 
> > > According to Stephan in this discussion:
> > > 
> > > https://www.mail-archive.com/lyx-devel@lists.lyx.org/msg196176.html
> > > 
> > > it is unfortunately not possible to use thread_local on Mac before some
> > > time, it seems. I would actually be happy to hear the contrary.
> > 
> > Note that I also added the check for __cplusplus < 201103L, which should
> > assure full compliance with C++11.
> 
> Only in a perfect world. Note also that for clang you are always taking
> the first branch so the value of __cplusplus is irrelevant. If
> intentional, I suggest that you make it clear in the comment.

This is not important. Either branches would do. If you think that
clang deserves an exception, you can add it.

> > If thread_local is not supported, the
> > compiler should not set __cplusplus to such value or higher and thus
> > should use the fallback definition.
> 
> Sorry, this does not make sense to me. First, all of your definitions
> use some form of thread-local storage. The problem referred to above is
> that there is no implementation of it at all, even called __thread or
> something else. You do not acknowledge that there is an issue, so it
> is not clear to me that you have read the discussion mentioned above
> carefully. To the best of our knowledge, in the best case your current
> patch requires a discussion about dropping the support of Xcode < 8.
 
No, I don't see anything about the non existence of __thread. Instead,
I see that for gcc < 4.8 this existence is guaranteed. No report has
been received so far about compiling problems. If they should come in,
they will be properly addressed.
 
> > > (Also, it had been decided that LyX requires msvc ≥ 2015 so the second
> > > branch would not be necessary.)
> > 
> > I don't think that LyX really cannot be compiled with older versions.
> > Are there reports in this regard?
> > 
> 
> There was a discussion at
> 
> and parent messages, and maybe someplace else as well. Since my patch on
> Unicode string literals alluded to in that message is not going to be
> committed before 2.4 at least, it could make sense to support MSVC 2013
> in 2.3 if it currently works. What worries me most is lack of support
> for 
> ("magic statics") for which your criteria “does it compile?” is not
> going to be enough.

So, I don't see any problem with the second branch.

-- 
Enrico

Re: [LyX/2.2.x] Fix bugs #9598 and #10650

[Enrico Forestieri]

On Tue, Jun 20, 2017 at 02:45:19AM +0200, Guillaume MM wrote:

> Le 08/06/2017 à 02:07, Enrico Forestieri a écrit :
> > On Thu, Jun 08, 2017 at 12:50:19AM +0200, Guillaume MM wrote:
> > 
> > > Le 05/06/2017 à 23:15, Enrico Forestieri a écrit :
> > > > commit 59c22bd7b604a3ba9e0e78f7c51cb601f08d0192
> > > > Author: Enrico Forestieri
> > > > Date:   Mon Jun 5 23:14:48 2017 +0200
> > > > 
> > > >   Fix bugs #9598 and #10650
> > > > ---
> > > 
> > > > +// gcc < 4.8.0 and msvc < 2015 do not support C++11 thread_local
> > > > +#if defined(__GNUC__) && ((__GNUC__ == 4 && __GNUC_MINOR__ < 8) || 
> > > > __cplusplus < 201103L)
> > > > +#define THREAD_LOCAL_STATIC static __thread
> > > > +#elif defined(_MSC_VER) && ((_MSC_VER < 1900) || __cplusplus < 201103L)
> > > > +#define THREAD_LOCAL_STATIC static __declspec(thread)
> > > > +#else
> > > > +#define THREAD_LOCAL_STATIC thread_local static
> > > > +#endif
> > > > +
> > > 
> > > 
> > > According to Stephan in this discussion:
> > > 
> > > https://www.mail-archive.com/lyx-devel@lists.lyx.org/msg196176.html
> > > 
> > > it is unfortunately not possible to use thread_local on Mac before some
> > > time, it seems. I would actually be happy to hear the contrary.
> > 
> > Note that I also added the check for __cplusplus < 201103L, which should
> > assure full compliance with C++11.
> 
> Only in a perfect world. Note also that for clang you are always taking
> the first branch so the value of __cplusplus is irrelevant. If
> intentional, I suggest that you make it clear in the comment.
> 
> > If thread_local is not supported, the
> > compiler should not set __cplusplus to such value or higher and thus
> > should use the fallback definition.
> 
> Sorry, this does not make sense to me. First, all of your definitions
> use some form of thread-local storage. The problem referred to above is
> that there is no implementation of it at all, even called __thread or
> something else. You do not acknowledge that there is an issue, so it
> is not clear to me that you have read the discussion mentioned above
> carefully. To the best of our knowledge, in the best case your current
> patch requires a discussion about dropping the support of Xcode < 8.
> 
> 
> > 
> > > (Also, it had been decided that LyX requires msvc ≥ 2015 so the second
> > > branch would not be necessary.)
> > 
> > I don't think that LyX really cannot be compiled with older versions.
> > Are there reports in this regard?
> > 
> 
> There was a discussion at
> 
> and parent messages, and maybe someplace else as well. Since my patch on
> Unicode string literals alluded to in that message is not going to be
> committed before 2.4 at least, it could make sense to support MSVC 2013
> in 2.3 if it currently works. What worries me most is lack of support
> for 
> ("magic statics") for which your criteria “does it compile?” is not
> going to be enough.

Yes, you are right.

-- 
Enrico

Re: [LyX/2.2.x] Fix bugs #9598 and #10650

[Guillaume MM]

Le 08/06/2017 à 02:07, Enrico Forestieri a écrit :

On Thu, Jun 08, 2017 at 12:50:19AM +0200, Guillaume MM wrote:


Le 05/06/2017 à 23:15, Enrico Forestieri a écrit :

commit 59c22bd7b604a3ba9e0e78f7c51cb601f08d0192
Author: Enrico Forestieri
Date:   Mon Jun 5 23:14:48 2017 +0200

  Fix bugs #9598 and #10650
---



+// gcc < 4.8.0 and msvc < 2015 do not support C++11 thread_local
+#if defined(__GNUC__) && ((__GNUC__ == 4 && __GNUC_MINOR__ < 8) || __cplusplus 
< 201103L)
+#define THREAD_LOCAL_STATIC static __thread
+#elif defined(_MSC_VER) && ((_MSC_VER < 1900) || __cplusplus < 201103L)
+#define THREAD_LOCAL_STATIC static __declspec(thread)
+#else
+#define THREAD_LOCAL_STATIC thread_local static
+#endif
+



According to Stephan in this discussion:

https://www.mail-archive.com/lyx-devel@lists.lyx.org/msg196176.html

it is unfortunately not possible to use thread_local on Mac before some
time, it seems. I would actually be happy to hear the contrary.


Note that I also added the check for __cplusplus < 201103L, which should
assure full compliance with C++11.


Only in a perfect world. Note also that for clang you are always taking
the first branch so the value of __cplusplus is irrelevant. If
intentional, I suggest that you make it clear in the comment.


If thread_local is not supported, the
compiler should not set __cplusplus to such value or higher and thus
should use the fallback definition.


Sorry, this does not make sense to me. First, all of your definitions
use some form of thread-local storage. The problem referred to above is
that there is no implementation of it at all, even called __thread or
something else. You do not acknowledge that there is an issue, so it
is not clear to me that you have read the discussion mentioned above
carefully. To the best of our knowledge, in the best case your current
patch requires a discussion about dropping the support of Xcode < 8.





(Also, it had been decided that LyX requires msvc ≥ 2015 so the second
branch would not be necessary.)


I don't think that LyX really cannot be compiled with older versions.
Are there reports in this regard?



There was a discussion at

and parent messages, and maybe someplace else as well. Since my patch on
Unicode string literals alluded to in that message is not going to be
committed before 2.4 at least, it could make sense to support MSVC 2013
in 2.3 if it currently works. What worries me most is lack of support
for 
("magic statics") for which your criteria “does it compile?” is not
going to be enough.

Re: [LyX/2.2.x] Fix bugs #9598 and #10650

[Enrico Forestieri]

On Thu, Jun 08, 2017 at 12:50:19AM +0200, Guillaume MM wrote:

> Le 05/06/2017 à 23:15, Enrico Forestieri a écrit :
> > commit 59c22bd7b604a3ba9e0e78f7c51cb601f08d0192
> > Author: Enrico Forestieri
> > Date:   Mon Jun 5 23:14:48 2017 +0200
> > 
> >  Fix bugs #9598 and #10650
> > ---
> 
> > +// gcc < 4.8.0 and msvc < 2015 do not support C++11 thread_local
> > +#if defined(__GNUC__) && ((__GNUC__ == 4 && __GNUC_MINOR__ < 8) || 
> > __cplusplus < 201103L)
> > +#define THREAD_LOCAL_STATIC static __thread
> > +#elif defined(_MSC_VER) && ((_MSC_VER < 1900) || __cplusplus < 201103L)
> > +#define THREAD_LOCAL_STATIC static __declspec(thread)
> > +#else
> > +#define THREAD_LOCAL_STATIC thread_local static
> > +#endif
> > +
> 
> 
> According to Stephan in this discussion:
> 
> https://www.mail-archive.com/lyx-devel@lists.lyx.org/msg196176.html
> 
> it is unfortunately not possible to use thread_local on Mac before some
> time, it seems. I would actually be happy to hear the contrary.

Note that I also added the check for __cplusplus < 201103L, which should
assure full compliance with C++11. If thread_local is not supported, the
compiler should not set __cplusplus to such value or higher and thus
should use the fallback definition.

> (Also, it had been decided that LyX requires msvc ≥ 2015 so the second
> branch would not be necessary.)

I don't think that LyX really cannot be compiled with older versions.
Are there reports in this regard?

-- 
Enrico

Re: [LyX/2.2.x] Fix bugs #9598 and #10650

[Guillaume MM]

Le 05/06/2017 à 23:15, Enrico Forestieri a écrit :

commit 59c22bd7b604a3ba9e0e78f7c51cb601f08d0192
Author: Enrico Forestieri
Date:   Mon Jun 5 23:14:48 2017 +0200

 Fix bugs #9598 and #10650
---



+// gcc < 4.8.0 and msvc < 2015 do not support C++11 thread_local
+#if defined(__GNUC__) && ((__GNUC__ == 4 && __GNUC_MINOR__ < 8) || __cplusplus 
< 201103L)
+#define THREAD_LOCAL_STATIC static __thread
+#elif defined(_MSC_VER) && ((_MSC_VER < 1900) || __cplusplus < 201103L)
+#define THREAD_LOCAL_STATIC static __declspec(thread)
+#else
+#define THREAD_LOCAL_STATIC thread_local static
+#endif
+



According to Stephan in this discussion:

https://www.mail-archive.com/lyx-devel@lists.lyx.org/msg196176.html

it is unfortunately not possible to use thread_local on Mac before some
time, it seems. I would actually be happy to hear the contrary.

(Also, it had been decided that LyX requires msvc ≥ 2015 so the second
branch would not be necessary.)