Stabilizing 4.0.x as 4.1.0 and removing ElapsedTime
125 visualizzazioni
Passa al primo messaggio da leggere
Neil Hodgson
21 apr 2018, 19:10:4021/04/18
a Scintilla mailing list
C++11 contains a portable implementation of high resolution timers in <chrono> so I would like to remove the Platform::ElapsedTime class and replace it with <chrono>. This reduces the amount of code required for a platform layer diminishing implementation and maintenance costs. Since <chrono> has some deeply nested types, a new class ElapsedPeriod encapsulates <chrono> with the same interface as Platform::ElapsedTime. ElapsedTime has been used mostly for debugging but it is also used to ensure Scintilla is responsive when idle styling is active.
The attached patch implements this change although I would like to go further and remove ElapsedTime from the Platform header. This will break external platform layers but they can either remove their implementation of ElapsedTime or define ElapsedTime locally if they really want.
The promotion of Sci::Position to 64-bits on 64-bit platforms has revealed many places where there were casts to int or similar, restricting APIs to only accessing the first 2GB of any large document, so there have been a series of about 30 commits to fix these and also just to reduce the number of casts. There are around 2000 casts in Scintilla and each could mean a difficult to diagnose problem when types are changed.
Some of the changes have been to use more natural types to eliminate casts. C++ like to use size_t for sizes which are 64-bit on 64-bit platforms so I would like to move towards using size_t or ptrdiff_t in more places including the platform interface. There may still need to be casting in platform code as, for example, the Win32 APIs for drawing text use in (32-bit) lengths instead of 64-bit lengths. Even better would be using std::string_view (C++17) for platform APIs as that bundles up the pointer and length.
The bidirectional code is still changing its impact on the platform interface. The simple case where right-to-left elements are included in left-to-right text seems well understood now but the opposite case where the document is predominantly right-to-left is less well defined. It may be possible to deliver just the changes needed for the first case in 4.1 and continue work on the second case without plans for stabilizing. Eventually this would be delivered in 5.x.
Neil
The attached patch implements this change although I would like to go further and remove ElapsedTime from the Platform header. This will break external platform layers but they can either remove their implementation of ElapsedTime or define ElapsedTime locally if they really want.
The promotion of Sci::Position to 64-bits on 64-bit platforms has revealed many places where there were casts to int or similar, restricting APIs to only accessing the first 2GB of any large document, so there have been a series of about 30 commits to fix these and also just to reduce the number of casts. There are around 2000 casts in Scintilla and each could mean a difficult to diagnose problem when types are changed.
Some of the changes have been to use more natural types to eliminate casts. C++ like to use size_t for sizes which are 64-bit on 64-bit platforms so I would like to move towards using size_t or ptrdiff_t in more places including the platform interface. There may still need to be casting in platform code as, for example, the Win32 APIs for drawing text use in (32-bit) lengths instead of 64-bit lengths. Even better would be using std::string_view (C++17) for platform APIs as that bundles up the pointer and length.
The bidirectional code is still changing its impact on the platform interface. The simple case where right-to-left elements are included in left-to-right text seems well understood now but the opposite case where the document is predominantly right-to-left is less well defined. It may be possible to deliver just the changes needed for the first case in 4.1 and continue work on the second case without plans for stabilizing. Eventually this would be delivered in 5.x.
Neil
Teodor Petrov
22 apr 2018, 05:13:3022/04/18
a scintilla...@googlegroups.com
On 04/22/2018 02:09 AM, Neil Hodgson wrote:
> Some of the changes have been to use more natural types to eliminate casts. C++ like to use size_t for sizes which are 64-bit on 64-bit platforms so I would like to move towards using size_t or ptrdiff_t in more places including the platform interface. There may still need to be casting in platform code as, for example, the Win32 APIs for drawing text use in (32-bit) lengths instead of 64-bit lengths. Even better would be using std::string_view (C++17) for platform APIs as that bundles up the pointer and length.
Just keep in mind that size_t is unsigned type and code using unsigned
> Some of the changes have been to use more natural types to eliminate casts. C++ like to use size_t for sizes which are 64-bit on 64-bit platforms so I would like to move towards using size_t or ptrdiff_t in more places including the platform interface. There may still need to be casting in platform code as, for example, the Win32 APIs for drawing text use in (32-bit) lengths instead of 64-bit lengths. Even better would be using std::string_view (C++17) for platform APIs as that bundles up the pointer and length.
types is more buggy
and often slower than code using signed types (compilers optimize better
code with signed
types, because signed types doesn't overflow). In general unsigned types
should be used only
for flags and when you're sure that you need the extra bit. So at least
consider using ssize_t
instead of size_t.
/Teodor.
Lex Trotman
22 apr 2018, 07:37:4922/04/18
a scintilla...@googlegroups.com
On 22 April 2018 at 19:14, Teodor Petrov <fusc...@gmail.com> wrote:
> On 04/22/2018 02:09 AM, Neil Hodgson wrote:
>>
>> Some of the changes have been to use more natural types to eliminate
>> casts. C++ like to use size_t for sizes which are 64-bit on 64-bit platforms
>> so I would like to move towards using size_t or ptrdiff_t in more places
>> including the platform interface. There may still need to be casting in
>> platform code as, for example, the Win32 APIs for drawing text use in
>> (32-bit) lengths instead of 64-bit lengths. Even better would be using
>> std::string_view (C++17) for platform APIs as that bundles up the pointer
>> and length.
>
>
> Just keep in mind that size_t is unsigned type and code using unsigned types
> is more buggy
Please provide supporting evidence for such a claim.
> On 04/22/2018 02:09 AM, Neil Hodgson wrote:
>>
>> Some of the changes have been to use more natural types to eliminate
>> casts. C++ like to use size_t for sizes which are 64-bit on 64-bit platforms
>> so I would like to move towards using size_t or ptrdiff_t in more places
>> including the platform interface. There may still need to be casting in
>> platform code as, for example, the Win32 APIs for drawing text use in
>> (32-bit) lengths instead of 64-bit lengths. Even better would be using
>> std::string_view (C++17) for platform APIs as that bundles up the pointer
>> and length.
>
>
> Just keep in mind that size_t is unsigned type and code using unsigned types
> is more buggy
> and often slower than code using signed types (compilers optimize better
> code with signed
> types, because signed types doesn't overflow).
overflow, but because the compiler is allowed to ASSUME they don't
overflow, its undefined behaviour if they do and the compiler can eat
your shorts. Unsigned types on the other hand have defined overflow
behaviour, so the compiler has to obey that and can't optimise away as
much.
But (subject to measurement of course) its unlikely that the
performance will be material for Scintilla, its not an integer math
heavy code.
In general unsigned types
> should be used only
> for flags and when you're sure that you need the extra bit. So at least
> consider using ssize_t
> instead of size_t.
numbers where negatives make no sense is huge.
Cheers
Lex
>
> /Teodor.
>
> --
> You received this message because you are subscribed to the Google Groups
> "scintilla-interest" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to scintilla-inter...@googlegroups.com.
> To post to this group, send email to scintilla...@googlegroups.com.
> Visit this group at https://groups.google.com/group/scintilla-interest.
> For more options, visit https://groups.google.com/d/optout.
Mike Lischke
22 apr 2018, 08:01:3122/04/18
a scintilla...@googlegroups.com
In general unsigned typesshould be used only
for flags and when you're sure that you need the extra bit. So at least
consider using ssize_t
instead of size_t.
On the other hand the semantic benefits of using unsigned types for
numbers where negatives make no sense is huge.
Additionally, there’s a reason why all of the container sizes in the STL and many other types are unsigned (also mostly because a size cannot be negative). When using signed types I find myself constantly casting between signed and unsigned types, which is a pain on itself (can easily hide problems). So I tend to use size_t where I can and only occasionally ssize_t.
Matthew Brush
22 apr 2018, 13:12:5822/04/18
a scintilla...@googlegroups.com
On 2018-04-22 04:37 AM, Lex Trotman wrote:
> On 22 April 2018 at 19:14, Teodor Petrov <fusc...@gmail.com> wrote:
>> On 04/22/2018 02:09 AM, Neil Hodgson wrote:
>>>
>>> Some of the changes have been to use more natural types to eliminate
>>> casts. C++ like to use size_t for sizes which are 64-bit on 64-bit platforms
>>> so I would like to move towards using size_t or ptrdiff_t in more places
>>> including the platform interface. There may still need to be casting in
>>> platform code as, for example, the Win32 APIs for drawing text use in
>>> (32-bit) lengths instead of 64-bit lengths. Even better would be using
>>> std::string_view (C++17) for platform APIs as that bundles up the pointer
>>> and length.
>>
>>
>> Just keep in mind that size_t is unsigned type and code using unsigned types
>> is more buggy
>
> Please provide supporting evidence for such a claim.
>
https://www.youtube.com/watch?v=Puio5dly9N8#t=42m40s
> On 22 April 2018 at 19:14, Teodor Petrov <fusc...@gmail.com> wrote:
>> On 04/22/2018 02:09 AM, Neil Hodgson wrote:
>>>
>>> Some of the changes have been to use more natural types to eliminate
>>> casts. C++ like to use size_t for sizes which are 64-bit on 64-bit platforms
>>> so I would like to move towards using size_t or ptrdiff_t in more places
>>> including the platform interface. There may still need to be casting in
>>> platform code as, for example, the Win32 APIs for drawing text use in
>>> (32-bit) lengths instead of 64-bit lengths. Even better would be using
>>> std::string_view (C++17) for platform APIs as that bundles up the pointer
>>> and length.
>>
>>
>> Just keep in mind that size_t is unsigned type and code using unsigned types
>> is more buggy
>
> Please provide supporting evidence for such a claim.
>
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es106-dont-try-to-avoid-negative-values-by-using-unsigned
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es107-dont-use-unsigned-for-subscripts-prefer-gslindex
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es102-use-signed-types-for-arithmetic
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es101-use-unsigned-types-for-bit-manipulation
https://github.com/ericniebler/stl2/issues/182
https://google.github.io/styleguide/cppguide.html#Integer_Types
Regards,
Matthew Brush
Teodor Petrov
22 apr 2018, 19:35:1322/04/18
a scintilla...@googlegroups.com
On 04/22/2018 03:00 PM, 'Mike Lischke' via scintilla-interest wrote:
> Additionally, there’s a reason why all of the container sizes in the
> STL and many other types are unsigned (also mostly because a size
> cannot be negative). When using signed types I find myself constantly
> casting between signed and unsigned types, which is a pain on itself
> (can easily hide problems). So I tend to use size_t where I can and
> only occasionally ssize_t.
Yes, they needed the extra bit when computers where 16 or 32 bits.
> Additionally, there’s a reason why all of the container sizes in the
> STL and many other types are unsigned (also mostly because a size
> cannot be negative). When using signed types I find myself constantly
> casting between signed and unsigned types, which is a pain on itself
> (can easily hide problems). So I tend to use size_t where I can and
> only occasionally ssize_t.
You can search for Bjarne's interviews where he admits this has been a
mistake.
/Teodor
Lex Trotman
22 apr 2018, 19:43:3822/04/18
a scintilla...@googlegroups.com
likely segfault, not happily read the location before the array and
keep going.
>
> /Teodor
Neil Hodgson
22 apr 2018, 23:18:2522/04/18
a scintilla...@googlegroups.com
Teodor Petrov:
> Just keep in mind that size_t is unsigned type and code using unsigned types is more buggy
> and often slower than code using signed types (compilers optimize better code with signed
> types, because signed types doesn't overflow).
That was why Sci_Position was defined as ptrdiff_t instead of size_t.
> In general unsigned types should be used only
> for flags and when you're sure that you need the extra bit. So at least consider using ssize_t
> instead of size_t.
For Sci_Position there were 3 candidates: ssize_t, ptrdiff_t, and intptr_t which are all signed integers that are commonly as large as machine addresses.
ssize_t is from POSIX, not the C or C++ standards so hasn’t been defined or been poorly defined by vendors who don’t really care about POSIX aka Microsoft. In some versions of Visual C++, ssize_t was defined as unsigned! Even with POSIX, ssize_t was only defined to include one negative number, -1, although this is unlikely to ever be implemented tightly:
“The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}].”
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html
Neil
> Just keep in mind that size_t is unsigned type and code using unsigned types is more buggy
> and often slower than code using signed types (compilers optimize better code with signed
> types, because signed types doesn't overflow).
> In general unsigned types should be used only
> for flags and when you're sure that you need the extra bit. So at least consider using ssize_t
> instead of size_t.
ssize_t is from POSIX, not the C or C++ standards so hasn’t been defined or been poorly defined by vendors who don’t really care about POSIX aka Microsoft. In some versions of Visual C++, ssize_t was defined as unsigned! Even with POSIX, ssize_t was only defined to include one negative number, -1, although this is unlikely to ever be implemented tightly:
“The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}].”
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html
Neil
Neil Hodgson
22 apr 2018, 23:19:4722/04/18
a Scintilla mailing list
As an example, the WidthText method on Surface currently looks like:
XYPOSITION WidthText(Font &font_, const char *s, int len);
Candidate replacements could be
XYPOSITION WidthText(Font &font_, const char *s, size_t len);
XYPOSITION WidthText(Font &font_, const char *s, ptrdiff_t len);
XYPOSITION WidthText(Font &font_, std::string_view text);
std::string_view is from the standard library and is basically { const char *ptr; size_t length; }.
I like string_view as I’ve been using it in simple parsing code in SciTE like the extended file pattern matcher and its safer than using bare pointers. It also implicitly converts from std::string and NUL-terminated strings.
An argument against string_view is that its an egregious use of C++17 causing unnecessary divergence.
Attached is a patch that implements std::string_view for the 6 Surface methods that take text arguments for both Win32 and GTK+. They both compile and run OK but hasn’t been properly tested.
Calls to the redefined methods are generally a little cleaner than the old code but some cases are worse and others are simplified quite well. An average change removes a cast but adds a class constructor:
widthSubLine = static_cast<int>(surface->WidthText(fontText,
- st.text + start, static_cast<int>(lenLine)));
+ std::string_view(st.text + start, lenLine)));
An example where the text is already in a std::string looks better:
rcNumber.left = rcNumber.right - surfaceMeasure->WidthText(
vsPrint.styles[STYLE_LINENUMBER].font,
- number.c_str(), static_cast<int>(number.length()));
+ number);
Neil
XYPOSITION WidthText(Font &font_, const char *s, int len);
Candidate replacements could be
XYPOSITION WidthText(Font &font_, const char *s, size_t len);
XYPOSITION WidthText(Font &font_, const char *s, ptrdiff_t len);
XYPOSITION WidthText(Font &font_, std::string_view text);
std::string_view is from the standard library and is basically { const char *ptr; size_t length; }.
I like string_view as I’ve been using it in simple parsing code in SciTE like the extended file pattern matcher and its safer than using bare pointers. It also implicitly converts from std::string and NUL-terminated strings.
An argument against string_view is that its an egregious use of C++17 causing unnecessary divergence.
Attached is a patch that implements std::string_view for the 6 Surface methods that take text arguments for both Win32 and GTK+. They both compile and run OK but hasn’t been properly tested.
Calls to the redefined methods are generally a little cleaner than the old code but some cases are worse and others are simplified quite well. An average change removes a cast but adds a class constructor:
widthSubLine = static_cast<int>(surface->WidthText(fontText,
- st.text + start, static_cast<int>(lenLine)));
+ std::string_view(st.text + start, lenLine)));
An example where the text is already in a std::string looks better:
rcNumber.left = rcNumber.right - surfaceMeasure->WidthText(
vsPrint.styles[STYLE_LINENUMBER].font,
- number.c_str(), static_cast<int>(number.length()));
+ number);
Neil
Neil Hodgson
25 apr 2018, 18:57:0725/04/18
a Scintilla mailing list
Me:
> C++11 contains a portable implementation of high resolution timers in <chrono> … a new class ElapsedPeriod encapsulates <chrono>
This change has been committed with all platform implementations of ElapsedTime removed. The class declaration is still in Platform.h for now but is deprecated and will be removed in a future release.
The code that measures performance of painting in EditView was changed to use #if instead of commenting out since there are several pieces of code that have to be enabled to display measurements.
Change set:
https://sourceforge.net/p/scintilla/code/ci/af5d9064c25c96ae8cc7e995da73aa890acc7000/
Neil
> C++11 contains a portable implementation of high resolution timers in <chrono> … a new class ElapsedPeriod encapsulates <chrono>
This change has been committed with all platform implementations of ElapsedTime removed. The class declaration is still in Platform.h for now but is deprecated and will be removed in a future release.
The code that measures performance of painting in EditView was changed to use #if instead of commenting out since there are several pieces of code that have to be enabled to display measurements.
Change set:
https://sourceforge.net/p/scintilla/code/ci/af5d9064c25c96ae8cc7e995da73aa890acc7000/
Neil
Neil Hodgson
3 mag 2018, 18:35:1203/05/18
a Scintilla mailing list
There are some more minor changes to Platform.h that may be worthwhile, making some methods more easily applicable, removing unnecessary methods, clarifying elements, and modernizing code. I’d like to make these changes in the release after next. The next release, 4.0.5, will have the 64-bit position support which is fairly major change.
Several data retrieval methods for positions and platform IDs are marked const as they are logically const and this allows some calling code to be simplified. Window::GetPosition, Window::GetClientPosition, Font::GetID.
Value classes like ColourDesired and Point have most methods marked noexcept. For resource-holding classes, some simpler methods are marked noexcept.
For resource-holding classes, the standard copy and assignment methods are all publicly defined, mostly with =delete so they cannot be used. This follows the C++ ‘rule of five’.
https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)
ColourDesired currently stores the colour in a long but long differs between Windows (32-bits) and Unix (64-bits). It needs 32-bits so is changed to int which is 32-bits on both platforms. A consequence of this is that the AsLong member is renamed to AsInteger. This change causes churn in calling code but retaining the name AsLong for something that returns an int appears too ugly to me.
The support for reading a ColourDesired from a string is only ever used for XPM images so that code was moved out of the header to the XPM implementation.
The Surface::WidthChar method is removed as it was only used in two places and can be replaced with calls to Surface::WidthText.
Font::SetID is protected as it should only be called from the FontAlias subclass.
Window::SetPositionRelative takes its Window argument as a const pointer to avoid copying. I’d like to go further with Window encapsulation as platform-independent code only needs very limited access with most window-handling code in the platform layers but haven’t found a good approach to this yet.
RoundXYPosition was removed as its inferior to the standard library round/roundf/lround functions.
The npts argument to Polygon changes from int to size_t for easier interop with standard library functions.
A patch with these changes is attached.
These changes will require changes to platform layer implementations.
Neil
Several data retrieval methods for positions and platform IDs are marked const as they are logically const and this allows some calling code to be simplified. Window::GetPosition, Window::GetClientPosition, Font::GetID.
Value classes like ColourDesired and Point have most methods marked noexcept. For resource-holding classes, some simpler methods are marked noexcept.
For resource-holding classes, the standard copy and assignment methods are all publicly defined, mostly with =delete so they cannot be used. This follows the C++ ‘rule of five’.
https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)
ColourDesired currently stores the colour in a long but long differs between Windows (32-bits) and Unix (64-bits). It needs 32-bits so is changed to int which is 32-bits on both platforms. A consequence of this is that the AsLong member is renamed to AsInteger. This change causes churn in calling code but retaining the name AsLong for something that returns an int appears too ugly to me.
The support for reading a ColourDesired from a string is only ever used for XPM images so that code was moved out of the header to the XPM implementation.
The Surface::WidthChar method is removed as it was only used in two places and can be replaced with calls to Surface::WidthText.
Font::SetID is protected as it should only be called from the FontAlias subclass.
Window::SetPositionRelative takes its Window argument as a const pointer to avoid copying. I’d like to go further with Window encapsulation as platform-independent code only needs very limited access with most window-handling code in the platform layers but haven’t found a good approach to this yet.
RoundXYPosition was removed as its inferior to the standard library round/roundf/lround functions.
The npts argument to Polygon changes from int to size_t for easier interop with standard library functions.
A patch with these changes is attached.
These changes will require changes to platform layer implementations.
Neil
Neil Hodgson
14 mag 2018, 06:32:0714/05/18
a Scintilla mailing list
The changes mentioned in previous messages to Platform.h have now been committed as change sets 6935 to 6949.
One change that didn’t work was making Font::SetID protected as it is called by ListBox code on Cocoa. A more comprehensive change to Font to clarify ownership and lifetimes would be worthwhile but its more complex than it appeared initially. There is already some fragility with fonts and the bidirectional work for Arabic exposed more areas of concern. Currently, the Style objects within ViewStyle are the true owners of fonts and other code may use them indirectly. A better approach could be to turn these into shared-owner objects using std::shared_ptr or providing a Font::Clone method to hand out new identical Fonts. Unsure whether this will be completed for this release or will be deferred until much later.
The LongTerm3 branch is likely to want all of the change sets except for 6936, 6937, 6941, and 6943 which use string_view which is C++17 only. Its possible the g++ releases targeted include this but Visual C++ 2015 definitely doesn’t.
Change set history - from [ec1d6c] Fix header order:
https://sourceforge.net/p/scintilla/code/ci/7747dc29d8a0a08e9cdf80758612d878bea76c0d/log/?path=
I’d like to make release 4.10 in around 4-5 weeks as the first stable 4.x release.
If there are any other changes wanted to Platform.h, they should be mentioned soon.
Neil
One change that didn’t work was making Font::SetID protected as it is called by ListBox code on Cocoa. A more comprehensive change to Font to clarify ownership and lifetimes would be worthwhile but its more complex than it appeared initially. There is already some fragility with fonts and the bidirectional work for Arabic exposed more areas of concern. Currently, the Style objects within ViewStyle are the true owners of fonts and other code may use them indirectly. A better approach could be to turn these into shared-owner objects using std::shared_ptr or providing a Font::Clone method to hand out new identical Fonts. Unsure whether this will be completed for this release or will be deferred until much later.
The LongTerm3 branch is likely to want all of the change sets except for 6936, 6937, 6941, and 6943 which use string_view which is C++17 only. Its possible the g++ releases targeted include this but Visual C++ 2015 definitely doesn’t.
Change set history - from [ec1d6c] Fix header order:
https://sourceforge.net/p/scintilla/code/ci/7747dc29d8a0a08e9cdf80758612d878bea76c0d/log/?path=
I’d like to make release 4.10 in around 4-5 weeks as the first stable 4.x release.
If there are any other changes wanted to Platform.h, they should be mentioned soon.
Neil
Neil Hodgson
7 giu 2018, 00:23:2107/06/18
a Scintilla mailing list
I would like to release Scintilla 4.1.0 in around 10 days, with the platform interface declared stable for the 4.x series.
If anyone wants changes made to Platform.h before stabilization then they should mention them now.
This stabilization refers to the definitions that have to be implemented by each platform and the minimum features provided to each platform. It will still be possible to add features that can be called by platform layers. Specifically, the IScreenLine interface implemented by core Scintilla and consumed by platform layers for bidirectional text may have methods added to improve functionality or efficiency.
Neil
If anyone wants changes made to Platform.h before stabilization then they should mention them now.
This stabilization refers to the definitions that have to be implemented by each platform and the minimum features provided to each platform. It will still be possible to add features that can be called by platform layers. Specifically, the IScreenLine interface implemented by core Scintilla and consumed by platform layers for bidirectional text may have methods added to improve functionality or efficiency.
Neil
Rispondi a tutti
Rispondi all'autore
Inoltra
0 nuovi messaggi