Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LexPascal: Styling improvement for preprocessor #298

Open
HoTschir opened this issue Dec 27, 2024 · 9 comments
Open

LexPascal: Styling improvement for preprocessor #298

HoTschir opened this issue Dec 27, 2024 · 9 comments
Labels
pascal Caused by the Pascal lexer

Comments

@HoTschir
Copy link
Contributor

Neil. Attached some styling improvement for LexPascal. Patch will grey out inactive preprocessor if/else block. Also test files are updated and a documentation is included.
"ASM styling" correction [link] is included. too.

I would ask you to take a look at it and check it out if you want to include it in Lexilla.

Regards HoTschir

lexilla.devLexPascal.3degaxtrcgmlemh7.patch.zip

@nyamatongwe nyamatongwe added the pascal Caused by the Pascal lexer label Dec 27, 2024
@nyamatongwe
Copy link
Member

I wrote a long reply but it seems to have been destroyed so here is a quick version.

That seems a good feature but there are some issues.

To prevent conflicts, all the contents except for the LexerModule definition should be inside an unnamed namespace like LexCPP.cxx. That allows static to be removed from function definitions. This should be done in a separate preparatory patch.

There are multiple setWord and setWordStart definitions and they differ slightly producing warnings and confusion. Best would be to rely on the file scope versions but if they have to differ, use different names.

SCE_PAS_PREPBLOCKINA should be more obvious like SCE_PAS_PREPROCESSORINACTIVE.

There are extraneous whitespace changes in the patch that obscure the intent and that should be reverted.

@zufuliu
Copy link
Contributor

zufuliu commented Dec 28, 2024

Maybe a property (e.g. lexer.pascal.track.preprocessor) to opt-in this feature?

@HoTschir
Copy link
Contributor Author

HoTschir commented Dec 30, 2024

Here is the replacement for patch above.
lexilla.devLexPascal.pmt52q2jupdczzna.patch.zip

According to the comments listed above, this patch includes:

  • Some refactoring done:
    - move functions into unnamed namespace
    - change name of setWord.. character sets to make same name have same content
    - introduce type alias Tokens
    - SCE_PAS_PREPBLOCKINA renamed to SCE_PAS_PREPROCESSORINACTIVE
  • For formatting, according to https://scintilla.org/SciCoding.html following command was used:
astyle --style=attach \
                  --indent=force-tab=8 \
                  --keep-one-line-blocks \
                  --pad-header \
                  --unpad-paren \
                  --pad-comma \
                  --indent-cases \
                  --indent-switches \
                  --align-pointer=name \
                  --pad-method-prefix \
                  --pad-return-type \
                  --pad-param-type \
                  --align-method-colon \
                  --pad-method-colon=after  LexPascal.cxx

For this patch, additionally some sed-filtering was done to remove extra spaces. I hope, it is fine now.

  • Introduce property lexer.pascal.style.preprocessor.inactive to opt-out the feature as follow:
        lexer.pascal.style.preprocessor.inactive=1   //to use style 15 for inactive block
        lexer.pascal.style.preprocessor.inactive=0   //to style inactive block same as active block
  • Update documentation
  • Update testfiles
  • Extra testfiles to verify correct work for both, lexer.pascal.style.preprocessor.inactive is set 0 or 1

@HoTschir
Copy link
Contributor Author

HoTschir commented Jan 1, 2025

OnTop here are new testfiles to verify correct work of properties:
lexilla.devLexPascal_PropertyTests.akee7rbizqb9kkop.patch.zip

Description:
Previously included test files make tests with the properties set to ON (1).
In order to check the correct function when a property is set to OFF (0), new
tests have been included. For this purpose, a subdirectory has been added for
each property, with the name of the property and appended "_0". This directory
contains a suitable Pascal source file and a modified SciTE.properties file in
which the property in question is set to OFF.

@HoTschir
Copy link
Contributor Author

HoTschir commented Jan 2, 2025

So far, here is my final LexPascal patch.
lexilla.devLexPascal.1753m8900podp7x2.patch.zip

It adds ability to use more keyword lists. There is one list named Tags (for common usage) or more lists for specific tag types like EnumTags, TypeTags, ConstTags, VarTags, ClassTags, FunctionTags and ProcedureTags. Also new/changed testfiles and documentation for new functionality is included.

Because, there are now more than 9 keyword lists in use, I had to slightly change TestLexers.cxx and also there is the need to adjust KEYWORDSET_MAX to at least 9 in Scintilla.h/Scintilla.iface. The latter is not content of this patch.

Keywords list naming introduced similar to style naming. For that, style names SCE_PAS_ were renamed to SCE_PAS_S_ and keywords list names start with SCE_PAS_K_ were added. Special definition SCE_PAS_K_LAST, which holds number of highest keywords list of lexer. Doing that, the handling of wordlists is more meaningful. Also SCE_PAS_K_LAST can be easily compared to KEYWORDSET_MAX while compiling. (Imperfection: LexicalStyles.iface may not be the correct place to handle keywords list definitions.)
If the latter is completely unwanted, I could move definition of SCE_PAS_K_ constants to LexPascal.cxx. However, then applications built on Lexilla can not work with them.
Regards HTs

@nyamatongwe
Copy link
Member

clPrepLineStates PrepLineStates; is a single instance that will be shared with each active Pascal file in the process causing interference and breakage. Similar features in other lexers are attached to a lexer object instance which is attached to a single file. This can only work for object lexers - see LexPython.cxx for an object lexer as it implements ILexer5 through DefaultLexer. LexPascal.cxx is a function lexer which was an earlier form before object lexers were added to allow extra per-file data. Other recent features only work on object lexers. For a recent example of the changes needed to convert a function lexer to an object lexer see 2b51b39.

When a test file is similar to other tests except for changing a simple option, its easier to use a match statement in SciTE.properties instead of a separate directory with most settings copied. See, for example, test/examples/hypertext/SciTE.properties.

The additional keyword lists appear to not be keywords but distinguished sets of identifiers. These should be defined as substyles - see LexCPP.cxx tracing from subStyles. Substyles allows applications (or users) to determine how many sets of distinguished identifiers they want for each base style.

The check_for_match_wl macro is enumerating the keyword lists to perform case-insensitive comparisons. A common technique in Lexilla is to lower case both the keyword lists and the search word. Substyles then uses std::map::find which is faster than a linear scan.

Changing KEYWORDSET_MAX is incompatible so can not be done until a new major version.

Changing style constant names in Lexilla.iface from SCE_PAS_ to SCE_PAS_S_ is a small incompatibility but does not appear to offer any benefit. The SCE_PAS_K_* constants aren't styles so should't use the SCE_ prefix or be in Lexilla.iface. The ILexer4::DescribeWordListSets and SCI_DESCRIBEKEYWORDSETS APIs can be used to discover keyword lists at run time.

nyamatongwe added a commit that referenced this issue Jan 4, 2025
Remove static since no longer needed.
@nyamatongwe
Copy link
Member

Committed a change to using an unnamed namespace and removing static.

@mpheath
Copy link

mpheath commented Jan 5, 2025

Should'nt lexer.pascal.style.preprocessor.inactive=0 be lexer.pascal.style.preprocessor.active=0?
I have not looked at the patches, just the descriptions in the posts. I guess it depends on the intent of the feature to be on or off by default.

Edit: Oh, I may have misinterpreted. to style inactive block same as active block is about active and inactive block rather than active as on or off. Even trying to describe it clearly gets a little confusing.

@nyamatongwe
Copy link
Member

Should'nt lexer.pascal.style.preprocessor.inactive=0 be ...

The cpp version is called lexer.cpp.track.preprocessor and that was copied for lexer.verilog.track.preprocessor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pascal Caused by the Pascal lexer
Projects
None yet
Development

No branches or pull requests

4 participants