Le 09/11/2011 02:46, Matthew Brush a écrit :
On 11/08/2011 02:55 PM, Colomban Wendling wrote:
[...]
These changes solve my biggest gripes with highlighting.c :) I *think* there might be some performance advantages to doing it this way too since all of the data is static/const and known upfront. I think the compiler can treat it differently now (but I'm not a GCC expert, so I could be wrong).
I doubt it, the compiler already knew the names (literals, so data) and styles (contsants, easy to optimize), and the code was inlined. Now it uses loops, and I doubt the compiler will inline them since they are used multiple times, so I guess there would be a small performance loss -- though I honestly doubt it's noticeable.
I'm no a GCC/ELF expert either, just my guess.
What's left to do is porting DocBook, HTML, PHP and XML.
For DocBook it is because it uses SCI_STYLESETEOLFILLED that my port don't support yet (could easily with one more column);
Is it possible to add the extra column, and just not use it in all the other arrays where it's not required?
Hehe, good idea! C implicitly initializes to 0/NULL/whateverfits missing members, so it's just fine. I did it for the already existing HLKeyword.merge, and used it for the HLStyle.fill_eol.
For HTML, PHP and XML it's a bit harder, because they use a weird mixed setup (that I didn't try to understand too deeply yet). IMO, first thing to do would be to make each have its own styles, and inherit XML's one in filetypes definitions ([styling=XML]) rather than in-code. There wouldn't be manual duplication with the proposed setup, just alias XML definition to HTML and PHP. Not sure what to do with the Python filetype dependency though.
I *hate* how all these languages are mixed into a single lexer :)
Unfortunately I won't fix this to make Scintilla call the appropriate lexer at the appropriate point in the appropriate file, and us set the appropriate styles at the appropriate moment [...] :-'
Anyway, I'd like your opinion on this, what to improve, what you feel good and bad about.
Heh, I actually did a tiny bit of work (mostly thinking) about basically the exact same thing. I have a script I wrote in Python that builds the lookup table/array code (nearly identical to yours) from Scintilla.iface, we could perhaps look at using that to re-generate the tables when Scintilla updates, or maybe even just for checking after updates. I don't know if it's necessary though now that you've done the bulk of the tables and they don't change *that* much between Scintilla versions. Just a thought.
It'd need adjustments from one side or another because there are a few "mappings" done in the code directly, like i.e. both SCE_D_TYPEDEF and SCE_D_WORD5 mapping to "typedef"; and as you said we anyway would need to update some code manually at some point.
But maybe it'd be interesting, maybe it'd help not to miss any new styles -- though actually checking the new Scintilla version's diff should be enough.
As we had discussed on IRC, I'd still rather not be storing duplicate name-mapping in (hard)code like this. It seems cleaner to me for example, to have the filetypes.* containing a '[style_map]' section which maps named styles to scintilla style/state numbers. The problem of course is that it would need comments everywhere that have the SCE_LANG_FOO constants, otherwise there would just be plain magic integers everywhere. I guess this approach is similar to SciTE properties files.
I agree we better not map twice, but I don't really like the plain integers like in SciTE .properties files -- even with the comments they aren't that readable. So... not sure.
Overall I think your changes are a great improvement in maintainability and readability (and maybe performance). Nice work!
Thanks :)
BTW, should we test your current repo or wait for the other filetypes to be implemented?
You could've done both, there was no point where a filetype stopped working. Now all are ported to the new system, so you can test without any more questions :)
BTW, I think XML/HTML/PHP might need more attention. Although I did my best and thin I didn't missed anything (and tried it a little), it changed the code far more than with the other filtypes, so maybe I *did* miss something.
Other filtypes should be just fine, it wasn't really hard to convert (C&P, plus replacing a few things -- just a little boring) and I doubt I made much mistakes. However, this doesn't mean testing is not useful, better not trust a big change like this before testing it :p
Cheers, Colomban