Rework

[theothornhill]

I think it may be time to merge this rework to master now.

it fixes #165 (obviously), #162, #154, #128, #114, #105, #84 and #46,

I think it is time to take a good look at it and see if something isn't in the correct basket. However, I don't think it is dangerous to merge to master stability-wise, since it now only relies on CC Mode and follows that api (apart from that one monkey patch that sneaked back in)

Should that new function be advised, or is it okay to just re-eval it here?

What do you think, @josteink?

[theothornhill]

Now most tests are passing on my machine... however, CI seems to choke on c-before-change-check-unbalanced-strings, which I find weird. Any thoughts?

Edit: Seems like it is added in emacs 27.1...

[josteink]

Now most tests are passing on my machine... however, CI seems to choke on c-before-change-check-unbalanced-strings, which I find weird. Any thoughts?

Edit: Seems like it is added in emacs 27.1...

My general approach to backwards compatibility has always been that csharp-mode should work on whatever version of Emacs was shipped with Debian Stable. From what I can tell right now that seems to be Emacs 26.1, but the CI is set to target a Emacs 24.4 minimum. It might be time to update the CI-matrix somewhat. 😄

With that said, if we are using/relying on a single Emacs 27.1 function in our code, changing the CI-setup won't fix our actual underlying problem... Could we maybe conditionally monkey patch that function too? Wouldn't that be enough?

[josteink]

Just FYI: I've now updated the CI in git master, and ... it actually breaks for Emacs 27.1 there, with the following test-failure:

So current master isn't perfect either in the current Emacs versions we should support.

[theothornhill]

Yeah, I think 24.4 is a very low version number.

My fix for that 27.1 function was actually not to declare it. We used the same as java anyways, so now it follows what java does for all versions. That seemed to fix it. I'm absolutely in favour of the debian support strategy, and think 26.1 should be enough. However, emacs is strict on backwards compatibility...

I think CC Mode should decide, and right now we follow CC Mode through history. I'd rather count csharp-mode overriding CC Mode defaults causing it to break a bug.

[theothornhill]

Ok, now only some fontification tests are failing.

I think (correct me if I'm wrong) that the most important here of them is the end of literal strings backslash test. That messes up Windows paths at least, right?

I don't know how to fix that with what CC mode offers. I think it is a bug in CC Mode actually. However, I'm not sure that what Alan sees as multiline strings also are literal strings. If we use \\ it fontifies correctly.

What do you think, @josteink?

[wasamasa]

This is handled by https://github.com/josteink/csharp-mode/blob/7cb8d0537a4e9c8a49714dae03948693abf6285b/csharp-mode.el#L1145-L1186.

[theothornhill]

This is handled by

Yeah, I know, but as mentioned here, I am very wary of adding this to csharp-mode.

However, if it is impossible to handle without, I guess we do need that code.

[wasamasa]

It isn't impossible, just the easiest solution I've found good code examples for at that time. What Alan proposes here is to use "either or both of c-get-state-before-change-functions and c-before-font-lock-functions", and indeed, it seems that cc-engine.el and cc-fonts.el contain non-trivial code to handle raw C++ strings. If you can make sense of that code, feel free to do better than me.

[josteink]

The current C# mode uses syntax-propertize-function. This clashes
nastily with CC Mode's use of the syntax-table text property. When
syntax-propertize-function is non-nil, Emacs arbitrarily wipes all
syntax-table properties from a region then relies on s-p-f to reapply
all the necessary s-t properties. However the current C# s-p-f only
applies a restricted subset of the necessary properties. This will
produce random bugs.

The documentation for syntax-propertize-function didn't used to mention
this problem, but it does now. (As from Emacs 27.2).

Reading it like that makes it seem like it might cause more issues than it solves? Not sure.

However, if it is impossible to handle without, I guess we do need that code.

If I had to chose, I would certainly prefer mostly working except multi-line strings (which is not used that often?), rather than random bugs throughout the major-mode, across different features.

[theothornhill]

If you can make sense of that code, feel free to do better than me.

I'm absolutely not saying I either can make sense, or do better - just wanted to heed his warnings. And as far as for inclusion in emacs, I'd guess not doing this would be a prerequisite.

Reading it like that makes it seem like it might cause more issues than it solves? Not sure.

I believe so yes, and this is also the reason I wanted to do this rewrite. If more and more of these creep in, I'm not sure we've won anything. However, if this is a real issue that needs to be solved before merging to master, then we should absolutely fix it. If not, I can add it to my todo-list, and try to make sense of things later on.

[josteink]

However, if this is a real issue that needs to be solved before merging to master, then we should absolutely fix it.

I'm just one vote in this whole Emacs-verse, but to me, the times I'm using multi-line strings is fairly limited. I wouldn't call it a pre-requisite to get things into master.

If anything, getting this into master should let us know the scope of the problem 🤠

[theothornhill]

However, if this is a real issue that needs to be solved before merging to master, then we should absolutely fix it.

I'm just one vote in this whole Emacs-verse, but to me, the times I'm using multi-line strings is fairly limited. I wouldn't call it a pre-requisite to get things into master.

Well, you have been here for a while, so I think your vote at least gets a deciding weight.

If anything, getting this into master should let us know the scope of the problem 🤠

I agree. We can always revert 🥴

Should I comment out failing tests, or let master build fail?

[josteink]

Should I comment out failing tests, or let master build fail?

I prefer the build to be green when we consider the release good.

If we decide that these features are not important enough to be a requirement to be in master, then we should comment out/disable the test-cases and tests for those features to, to keep the build green.

So yes, IMO:

• the fontification-of-literals-detects-end-of-strings can have the failing test-case with traling \" commented out.
• and fontification-of-regions can have the #region "a region comment can even look like this" test-case removed.

With those 2 aspects of those 2 test-cases taken out, the build should be green, and we should be good to merge :)

[theothornhill]

It's passing! However, I had to comment out the whole tests, not only the aspects.

It comes down to two things:

1. Compiler directives
2. The literal backslash.

What do you think? Should l work more on this, or create a new issue for the compiler directives thing afterwards?

[josteink]

It's passing! However, I had to comment out the whole tests, not only the aspects.

That shouldn't really be needed. Let me see if I can make them pass with some slight modifications.

[josteink]

Ok so quick reality-check:

• the fontification-of-regions test expects region-comments to have font-lock-comment-face, which they most certainly does not. So yeah, this whole test will have to go, for now. That's OK.
• looking into the specifics of fontification-of-literals-detects-end-of-strings, it has been a "valid" test-case for the previous code-base at detecting the string-literal aspect of the test. I see the other fontification aspects are not entirely proper. I also noticed (during manual inspection) another failure: const was treated like a type, not a keyword.

So with that said, I've fixed const behvaiour, and will I'll try to fix the remaining test, which I honestly think should pass.

[theothornhill]

Ok so quick reality-check:

* the `fontification-of-regions` test expects region-comments to have `font-lock-comment-face`, which they most certainly does not. So yeah, this whole test will have to go, for now. That's OK.

Yeah, This is the syntax-propteries-function issue. I know there are some regexes inside cc-langs that deal with cpp, but they are super complicated, so I need to figure out how to deal with this. Maybe I need to ask on emacs-devel (though they aren't very responsive to CC Mode related things)

* looking into the specifics of `fontification-of-literals-detects-end-of-strings`, it has been a "valid" test-case for the previous code-base at detecting the string-literal aspect of the test. I see the other fontification aspects are not entirely proper. I also noticed (during manual inspection) another failure: `const` was treated like a type, not a keyword.

So with that said, I've fixed const behvaiour, and will I'll try to fix the remaining test, which I honestly think should pass.

That is great! Thank you!

[josteink]

There. Done. (Assuming build goes green)

[theothornhill]

Yay, we are green. Thanks, all!

[josteink]

The continuous-integration/travis-ci/push build is probably going to keep failing as long as we don't have the latest .travis.yml merged into this branch.

Looking at the compatibility with previous versions of Emacs before 26.1... The test-suite seems to run fine for most aspects apart from the "big" indentation-test. That's not anywhere near being completely broken, and might even be usable for day-to-day work, so I think that's fine.

As long as continuous-integration/travis-ci/pr goes through, I'll be happy to merge. Aaand...

Good to go!

And at this point, it would be completely unfair for me to click the big, bad "merge"-button.

You rewrote this thing from scratch. You did all the heavy-lifting. You get the honours :)

The show is all yours @theothornhill !

[theothornhill]

Good to go!

That is fantastic! Thank you for all your help!!

And at this point, it would be completely unfair for me to click the big, bad "merge"-button.

You rewrote this thing from scratch. You did all the heavy-lifting. You get the honours :)

Thanks :)

The show is all yours @theothornhill !

oOo!

[josteink]

I'm not sure how many bugs this single effort has fixed, but the oldest one is like 5 years old, and you got it all done in about a week's worth of effort.

Absolutely amazing. You're a hero, man! 👍

[theothornhill]

I'm not sure how many bugs this single effort has fixed, but the oldest one is like 5 years old, and you got it all done in about a week's worth of effort.

I really hope keeping things simple will aid in this onwards! That CC Mode is complicated enough

Absolutely amazing. You're a hero, man! 👍

My pleasure! It was great fun :) I'll stick around and fix whatever bugs I caused :)

[josteink]

My pleasure! It was great fun :) I'll stick around and fix whatever bugs I caused :)

Oh you'll have great fun in the future too, have no worries. Bugs will always come to those who wait. 😁

As this is largely your code now... Do you have feelings wrt getting it mainlined?

[theothornhill]

Oh you'll have great fun in the future too, have no worries. Bugs will always come to those who wait. 😁

Haha! ✌️

As this is largely your code now... Do you have feelings wrt getting it mainlined?

I think it is worth the effort, absolutely. With the move towards lsp-mode and dotnet core and whatnot, editing C# with emacs may be more viable than ever. Even fewer moving parts would be a huge win, IMO.

However, CC Mode needs to adapt to modes not already in CC Mode, to avoid that sort of monkey patching.

So:

1. I would like to fix that compiler preprocessor thing. Not sure how to do it, but it may be doable with CC mode only
2. I think the string literal needs to be fixed in mainline
3. We really need to be able to define other syntactic constructs in CC Mode, like the one needed for lambda functions.

Also, it's not like C# is a more niche language than Simula and Modula 2 :)

[josteink]

Hehe. Sounds reasonable. No need to rush things 👍