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

Major revision (complete overhaul of nearly everything) #93

Merged
merged 146 commits into from
Aug 30, 2020

Conversation

CyberShadow
Copy link
Member

Fixes a lot of bugs and I'm sure introduces many others. WIP.

Uploading this now as I'm taking a break from this project to work on https://github.com/CyberShadow/TrenD.

@coveralls
Copy link

coveralls commented Aug 30, 2019

Coverage Status

Coverage increased (+2.5%) to 93.278% when pulling bc4864d on CyberShadow:next into cfd1d08 on Emacs-D-Mode-Maintainers:master.

@nordlow
Copy link
Contributor

nordlow commented Aug 30, 2019

Great work.

@CyberShadow CyberShadow force-pushed the next branch 2 times, most recently from 7104dc8 to f40e317 Compare September 5, 2019 11:52
@nordlow
Copy link
Contributor

nordlow commented Sep 6, 2019

Is this ready to go?

@CyberShadow
Copy link
Member Author

No. There's still some things on my personal TODO that I want to fix. After that, I'd like to use it for a few days to try to spot regressions before pushing it to master. Help with testing what's here so far would be helpful.

@nordlow
Copy link
Contributor

nordlow commented Sep 8, 2019

Is it possible to fontify parens-const-qualified function parameters and return types such as

const(Type) foo(const(Type) param);

? Currently first occurrence of Type, foo and param are not fontified.

@CyberShadow
Copy link
Member Author

Anything is possible, file some issues and we can see if it's feasible.

@CyberShadow
Copy link
Member Author

Hierarchical imenu:

screenshot

@CyberShadow
Copy link
Member Author

That's everything from my list.

@CyberShadow CyberShadow marked this pull request as ready for review September 12, 2019 03:46
@CyberShadow
Copy link
Member Author

CyberShadow commented Sep 12, 2019

Summary of changes:

  • Fixes all remaining open issues, except DDoc support.
  • Further extends cc-mode's understanding of D syntax, fixing many fontification and indentation issues.
  • A new imenu implementation (for Emacs 26+) which uses cc-mode's parsing instead of regular expressions. It also arranges entries in a tree (like an outliner) shows the "fully-qualified" name for declarations according to their context in the D source code.

@russel

Pertinent project-wide changes:

  • Dropped support for Emacs 24. Emacs 26 is now officially supported.
  • The code now includes some bits of (modified) cc-mode code (as I couldn't find a way to implement some D constructs through only configuration and advice micro-patching).
  • The license is now GPLv3 or newer, same as Emacs and cc-mode.
  • Probably this should be tagged as v2.1 once this is merged and stabilized.

@atilaneves @MartinNowak @andralex @dmakarov

If you or anyone would like to test this early, or have any feedback about the implementation or other suggestions, now would be a good time.

My plan is to merge this into master one week-ish from now or the last fix for any regressions found if any. After that and a while more, we can tag a stable release.

One possible future direction would be to work with cc-mode developers to implement some hooks in cc-mode to allow supporting d-mode in a way that relies less on cc-mode implementation details (or copying code), though I don't know how enthusiastic they'd be about the idea.

@andralex
Copy link

andralex commented Sep 12, 2019 via email

@atilaneves
Copy link

I'm just going to trust you on this one. Good work!

@nordlow
Copy link
Contributor

nordlow commented Oct 11, 2019

@CyberShadow:

Found the cornercase

struct S
{
    @safe:
    T x;
}

in which T x doesn't fontify, regardless of indentation of @safe:.

@CyberShadow
Copy link
Member Author

I actually found a lot of bugs / regressions. I'll add this to the pile :)

I hope I can get back to this as soon as I finish my current project, which is taking a bit longer than expected.

@nordlow
Copy link
Contributor

nordlow commented Oct 27, 2019

Found one more cornercase:

Parameters qualified with return doesn't fontify type and name as in, for instance,

void f(T)(scope return T x)
{
}

when

void f(T)(scope T x)
{
}

works.

This is easily fixed by adding return to the definition of c-modifier-kwds as in

(c-lang-defconst c-modifier-kwds
  d '("abstract" "deprecated" "extern"
      "final" "out" "lazy" "mixin" "override" "private"
      "protected" "public" "ref" "return" "scope" "static" "synchronized"
      "volatile" "__vector"))

@nordlow
Copy link
Contributor

nordlow commented Nov 10, 2019

Thanks for the fixes.

This case remains to correctly fontify C:

@safe:
class C { }

.

@nordlow
Copy link
Contributor

nordlow commented Jan 9, 2020

Qualifier package on class-definitions doesn't seem to fontify the class symbol correctly, such as in

package class A {}
package abstract class B {}

@CyberShadow
Copy link
Member Author

Sorry, I can't reproduce the problem:

CyberShadow@e69366f

Works fine for me.

@nordlow
Copy link
Contributor

nordlow commented Jan 12, 2020

Sorry, I can't reproduce the problem:

CyberShadow@e69366f

Works fine for me.

Strange. What Emacs version are you using? I'm currently sitting on Emacs master.

@CyberShadow
Copy link
Member Author

26.3.

@CyberShadow
Copy link
Member Author

I added the test case to this branch for CI, and it passed for all supported Emacs versions:

https://travis-ci.org/Emacs-D-Mode-Maintainers/Emacs-D-Mode/builds/637279679

@nordlow
Copy link
Contributor

nordlow commented Jan 15, 2020

I put any more time into this for a while. Thanks.

@nordlow
Copy link
Contributor

nordlow commented Feb 1, 2020

Does this fontify correctly for you?

alias Expr = string[];

void toString(scope void delegate(scope const(char)[]) @safe sink,
              scope const Expr expr);

I get

Error during redisplay: (jit-lock-function 25) signaled (scan-error "Unbalanced parentheses" 38 101)

during fontification.

The Expr part of the second argument doesn't fonity at all.

@CyberShadow
Copy link
Member Author

The Expr part of the second argument doesn't fonity at all.

Same here (because of @safe), but I don't get the error.

@nordlow
Copy link
Contributor

nordlow commented Feb 1, 2020

The Expr part of the second argument doesn't fonity at all.

Same here (because of @safe), but I don't get the error.

Ok, thanks. I'll see if I can figure out the source of the error.

@CyberShadow
Copy link
Member Author

Correction, I do see the error if I look in *Messages*.

@nordlow
Copy link
Contributor

nordlow commented Feb 1, 2020

Correction, I do see the error if I look in Messages.

Thanks.

@CyberShadow
Copy link
Member Author

Alan Mackenzie, the maintainer of CC Mode, contacted Russell and me today regarding one use of Emacs Lisp advice in the current (master) version of d-mode:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=18158#26

As the changes in this PR introduce additional advice patches such as the one fixed by Alan's patch, I replied inquiring about the possible future for d-mode going forward. My reply is below Alan's message at the link above.

Doesn't seem to be necessary any more.
Revert an earlier fontification fix ("Fix fontification of lone
protection labels") to fix indentation regression.
@Geod24
Copy link

Geod24 commented Jul 15, 2020

Any chance some of this could make it to the repository ? I'm particularly interested in 1ec5880 as it fixes #90 which makes working on expression.d quite painful...

@CyberShadow
Copy link
Member Author

I think at this point it would have to be a fork or some kind of branch, because 1) it's much more difficult to maintain, and 2) it introduces its own regressions (which are perhaps less severe than the ones in master). Unfortunately we didn't get anywhere in the discussion with the cc-mode maintainer either.

@Geod24
Copy link

Geod24 commented Aug 20, 2020

I've been using it daily for a month now, and bar for the little bug when writing function prototypes, it really works well.

@nordlow
Copy link
Contributor

nordlow commented Aug 21, 2020

I agree.

@CyberShadow
Copy link
Member Author

Should we just merge this? Or maybe publish it as a separate package?

The biggest concern is that this version depends a lot of cc-mode internals, and if a future Emacs version breaks it, it might not be easy to fix again.

@Geod24
Copy link

Geod24 commented Aug 22, 2020

I think a separate package would be confusing and divert effort. I'd be in favor of just merging it, as mentioned earlier. And perhaps now is the time to ping the maintainer again ?

@CyberShadow
Copy link
Member Author

CyberShadow commented Aug 30, 2020

I'd be in favor of just merging it, as mentioned earlier.

OK.

And perhaps now is the time to ping the maintainer again ?

It's possible that we might get to a better place through collaboration, but that requires some expended time and effort from both sides. It does seem to me that time would be instead better spent on solutions based on language servers, so that all editors benefit from them, and we don't have to implement the same functionality once per editor.

Edit: Just noticed this pull request was exactly one year old today :)

@CyberShadow CyberShadow changed the title Bunch of fixes Major revision (complete overhaul of nearly everything) Aug 30, 2020
@CyberShadow CyberShadow merged commit 447515f into Emacs-D-Mode-Maintainers:master Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants