Don't crash on malformed syntax highlighting files
ClosedPublic

Authored by davispuh on Mar 5 2019, 12:07 AM.

Details

Test Plan
  1. Create malformed syntax highlighting file with missing end tags
  2. Place it in ~/.local/share/katepart5/syntax/
  3. Open Kate with file which uses that syntax highlighting

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davispuh created this revision.Mar 5 2019, 12:07 AM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMar 5 2019, 12:07 AM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
davispuh requested review of this revision.Mar 5 2019, 12:07 AM
davispuh updated this revision to Diff 53235.Mar 5 2019, 8:24 PM

Allow invalid Format()

davispuh updated this revision to Diff 53236.Mar 5 2019, 8:26 PM

Whoops...

In principle ok, but I would like to have the m_properties.resi.... code in that if shared.
We already have that, just a few lines above.
You just need to move the computation of the auto definitions = definition().includedDefinitions(); a few lines up, you can just call that on the passed def.

cullmann requested changes to this revision.Mar 5 2019, 8:28 PM
This revision now requires changes to proceed.Mar 5 2019, 8:28 PM

And btw., thanks for taking care of that crash, it's ugly to crash for such things. My fault, did never test that :/

davispuh updated this revision to Diff 53237.Mar 5 2019, 8:38 PM

Don't duplicate code

Hmm, I just thought that might be correct, but doesn't that kill highlighting for all definitions that include none?
If forgot that includedDefinitions() doesn't include itself, that check makes no sense ;=)

I think we are missing some check at the end if we had no formats at all.

cullmann requested changes to this revision.Mar 5 2019, 8:44 PM

Sorry that I was confused, but I just have seen your check of definitions.isEmpty() and thought that would be the right one.

This revision now requires changes to proceed.Mar 5 2019, 8:44 PM
davispuh updated this revision to Diff 53970.Mar 15 2019, 5:43 PM

Also check for present formats

cullmann accepted this revision.Mar 16 2019, 3:31 PM

That looks reasonable, if the definition doesn't have any included ones + no own formats it is likely to be broken ;=)

This revision is now accepted and ready to land.Mar 16 2019, 3:31 PM
This revision was automatically updated to reflect the committed changes.