implicit fallthough if there is fallthoughContext

Authored by jpoelen on Aug 9 2019, 12:31 AM.



fallthrough="true" is redundant with fallthroughContext="xxx" and there is no reason to use fallthrough="false"

Diff Detail

R216 Syntax Highlighting
auto_fallthrough (branched from master)
No Linters Available
No Unit Test Coverage
Build Status
Buildable 14933
Build 14951: arc lint + arc unit
jpoelen requested review of this revision.Aug 9 2019, 12:31 AM
jpoelen created this revision.
cullmann requested changes to this revision.Aug 9 2019, 5:19 AM

I appreciate the removal of the fallthrough=bool attribute.
Given it is sematic-free if no fall through context is there I guess this is a "compatible" change.
But please update the XSD (there are even some comments about fallthrough being dubious)

This revision now requires changes to proceed.Aug 9 2019, 5:19 AM
dhaumann requested changes to this revision.Aug 9 2019, 7:33 AM

I like this change a lot, but there is a huge (theoretical) backwards compatibility issue with this.

Before, the code ignored fallthroughContext if m_fallthrough was explicitly set to false. So you could have: fallghrough="false" fallthroughContext="abc", and fallthroughContext was ignored.
Now this changed, and m_fallthough solely depends on whether fallthroughContext is a valid context or not other than #stay.

My point is: For changes like this, we usually have to

  • increase the version number in the *.xml files
  • increase the kateversion from 5.0 to 5.62?
  • Since we have IncludeRules, wouldn't we even need to increase the kateversion number of _all_ xml files to be on the safe side?

I think these issues first need to be clarified before we can commit this.

Hmm, isn't any file with some fallthrough=false but a fallthrough context just buggy? Did any of our files have that?

nibags added a subscriber: nibags.Aug 9 2019, 3:47 PM

I think it's an improvement, but the ideal would be to maintain support for fallthrough="bool". That is, if the fallthroughContext attribute is present, assume fallthrough="true", unless "fallthrough=" is explicitly written.

What I don't agree with is modifying all *.xml files, since this implies using kateversion="5.62", which will generate problems for those users who install the *.xml files manually in previous versions of KDE Frameworks (and not all GNU/Linux distributions have the latest version). This, considering that this is just a "cosmetic" change in the code of the *.xml files.

So, I propose two alternatives:

  • Maintain support for fallthrough="bool", as an optional attribute (i.e., what I wrote at the beginning), as a transition and in a future version, such as KF6, completely eliminate the "fallthrough" attribute, since as jpoelen says this is redundant.
  • Now remove support for the "fallthrough" attribute (what jpoelen proposes), but don't modify the *.xml files to maintain the compatibility of these files in older versions of KF5. No *.xml files uses fallthrough="false" or "fallthroughContext" alone, so this doesn't cause regression. In that case, it should be announced in the documentation that this attribute is obsolete.

Sounds like a reasonable solution.

I will hand over the original xml files, that seems preferable to me too.

I did not see fallthrough="false" nor fallthrough="0". But after checking, brightscript.xml uses` fallthroughContext` twice without fallthrough. in my opinion, it's an oblivion with no unfortunate consequences.

jpoelen updated this revision to Diff 63455.Aug 10 2019, 1:01 AM
  • don't modify the *.xml files to maintain the compatibility in older versions of KF5
  • langauge.xsd: accept only 1 or true for fallthrough attribute
dhaumann accepted this revision.Aug 10 2019, 5:42 AM

I think this is much better. For KF6, we can remove it in the xml files as well or so...

Besides that: why does the diff show so many changes even though it's just about fall through? It makes the review harder (especially on a smartphone).

Some whitespace stuff, git diff -w looks OK.

cullmann accepted this revision.Aug 13 2019, 8:45 PM
This revision is now accepted and ready to land.Aug 13 2019, 8:45 PM
cullmann closed this revision.Aug 13 2019, 8:45 PM