This repository has been archived by the owner on Jan 13, 2024. It is now read-only.
[x64 fix]: Scintilla structures required correction for x64 #75
+507
−438
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
mahee96
commented
May 26, 2021
mahee96
commented
- ScNotification struct had incorrect data type 'int' for lineNumber, position, etc fields
- This might cause misinterpretation of data block eventually leading to corruption.
- IntPtr must have been used.
- Scintilla interface method signatures were incorrectly using int.
- This would cause truncation of data in x64 environment and possible data corruption.
- Enums in GatewayDomain.cs required [Flags] attribute to combine using OR.
- ScNotification struct had incorrect data type 'int' for lineNumber, position, etc fields - This might cause misinterpretation of data block eventually leading to corruption. - Scintilla interface method signatures were incorrectly using int. - This would cause truncation of data in x64 environment and possible data corruption. - Enums in GatewayDomain.cs required [Flags] attribute to combine using OR.
mahee96
commented
May 26, 2021
| @@ -50,6 +50,10 @@ def ReadFromFile(self, name): | |||
| currentCategory = "" | |||
| currentComment = [] | |||
| currentCommentFinished = 0 | |||
| import os, os.path, inspect | |||
| if '__file__' not in locals(): | |||
| __file__ = inspect.getframeinfo(inspect.currentframe())[0] | |||
There was a problem hiding this comment.
set cwd to script path for traversing relative file paths correctly such as ../../ etc
mahee96
commented
May 26, 2021
| if t == "cells": return "Cells" | ||
| if t == "colour": return "Colour" | ||
| if t == "line": return "int" | ||
| if t == "line": return "long" |
There was a problem hiding this comment.
line and position types need to be atleast 64-bit wide which can accommodate both 32-bit data in x86 execution and 64-bit data in x64 execution.
Hence long is used instead of int.
IntPtr could have been used as suggested, but that complicates things with arithmetic operations.
There was a problem hiding this comment.
is this the correct assumption if you build a 32bit and a64 bit version
There was a problem hiding this comment.
you are right, there is a potential issue for set operations, if we allow the long to be used and user supplies a long value but the execution env is x86. This might result in overflow/truncation.
I was just focusing on get operation where the system returned type can fit on the target container for both x86 and x64.
So we are again left with only option of using IntPtr to make it work for both environment.
There was a problem hiding this comment.
I have suggested alternative fix for this: see my reply below
mahee96
commented
May 26, 2021
| @@ -100,6 +100,7 @@ def printEnumDefinitions(f): | |||
| if v["FeatureType"] in ["enu"] and name not in ["Keys"]: # for all except excluded enums [conflicting] | |||
| appendComment(indent, out, v) | |||
| prefix = v["Value"] | |||
| out.append(indent + "[Flags]") | |||
There was a problem hiding this comment.
Enums now have the [Flags] Attribute which makes them combinable with other constants of same enum type.
mahee96
commented
May 26, 2021
| /// <summary>Is undo history being collected? (Scintilla feature SCWS_)</summary> | ||
| [Flags] |
There was a problem hiding this comment.
[Flags] attribute inclusion for enum types
mahee96
commented
May 26, 2021
| @@ -34,40 +34,40 @@ public interface IScintillaGateway | |||
|
|
|||
| /* ++Autogenerated -- start of section automatically generated from Scintilla.iface */ | |||
| /// <summary>Add text to the document at current position. (Scintilla feature 2001)</summary> | |||
| unsafe void AddText(int length, string text); | |||
There was a problem hiding this comment.
'long'(64-bits wider) type is much flexible than 'int'/'intPtr'(32 bits/64 bits speculative) in x86/x64.
Keeps the interface clean without requiring much changes due to change in data type.
mahee96
commented
May 26, 2021
| @@ -47,7 +47,7 @@ public void InsertTextAndMoveCursor(string text) | |||
|
|
|||
| public void SelectCurrentLine() | |||
| { | |||
| int line = GetCurrentLineNumber(); | |||
There was a problem hiding this comment.
Same: using 'long' instead of 'int' for param and return types.
mahee96
commented
May 26, 2021
| @@ -38,31 +38,32 @@ public struct ScNotificationHeader | |||
| public struct ScNotification | |||
| { | |||
| public ScNotificationHeader Header; | |||
| private int position; /* SCN_STYLENEEDED, SCN_DOUBLECLICK, SCN_MODIFIED, SCN_MARGINCLICK, SCN_NEEDSHOWN, SCN_DWELLSTART, SCN_DWELLEND, SCN_CALLTIPCLICK, SCN_HOTSPOTCLICK, SCN_HOTSPOTDOUBLECLICK, SCN_HOTSPOTRELEASECLICK, SCN_INDICATORCLICK, SCN_INDICATORRELEASE, SCN_USERLISTSELECTION, SCN_AUTOCSELECTION */ | |||
| private IntPtr position; /* SCN_STYLENEEDED, SCN_DOUBLECLICK, SCN_MODIFIED, SCN_MARGINCLICK, SCN_NEEDSHOWN, SCN_DWELLSTART, SCN_DWELLEND, SCN_CALLTIPCLICK, SCN_HOTSPOTCLICK, SCN_HOTSPOTDOUBLECLICK, SCN_HOTSPOTRELEASECLICK, SCN_INDICATORCLICK, SCN_INDICATORRELEASE, SCN_USERLISTSELECTION, SCN_AUTOCSELECTION */ | |||
There was a problem hiding this comment.
ScNotification struct alignment and data type size issues fixes
mahee96
added
bug-fix
This was
linked to
issues
May 26, 2021
|
Some screenshots of states and memory view of variables.
|
|
@kbilsted Here is an attempt to resolve this issue using wrapper classes: Changes to be made in source files to accommodate this:
After: Caveats:
Advantages:
Other methods:
But our goal here should be that we should not often change the interface signature, so in that case I would vote for the wrapper classes method, which allows us(the maintainers) to make changes in the wrapper classes as required without changes to the interface signatures and wrapper class name itself. |
|
@kbilsted, Found another code where scintilla.iface content has been generated for cpp as ScintillaEdit.h: Note the sptr_t which is ptrdiff_t or _int/_int64 based on platform type, equivalent would be IntPtr |
This was referenced Feb 6, 2022
|
Closing this PR since making x86 and x64 code co-exist is non-trivial. Will deal with it in separate PR |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.