lsp-mode icon indicating copy to clipboard operation
lsp-mode copied to clipboard

Add syntax table property for comment semantic tokens.

Open JimDBh opened this issue 3 years ago • 6 comments

This is important to let emacs understand that certain parts of the code (e.g. wrapped in C marcos) are not in effect and should be by-passed for things like forward-sexp.

For example, if we have the following code in emacs with c-mode:

#include <stdio.h>

#define SOME_MACRO 1

int myFun(int par) { // => This bracelet has no matching
  // ...
#if SOME_MACRO
  if (par == 0) { // => This incorrectly matches with the bracelet at the end of function
    printf("example check 0\n");
#else
  if (par == 1) { // => This should have been ignored by forward-sexp
    printf("example check 1\n");
#endif
    // ...
  }
  return 0;
} // => this bracelet matches with the wrong one

void someFun() {
  // Do something.
  myFun(1);
  return; // => beginning-of-defun won't work here
}

With this patch, these should work fine.

JimDBh avatar Aug 26 '22 17:08 JimDBh

@sebastiansturm @ericdallo - willing to review this one?

yyoncho avatar Aug 26 '22 18:08 yyoncho

sorry for being late, have been away for a week. The feature looks very useful, though I'm not sure how to test it. I've set lsp-semantic-tokens-set-comment-syntax to t, and lsp-semantic-tokens--put-comment-syntax is being called, but comment-search-forward fails to return a match (in a small C++ buffer with an ifdef-disabled part that correctly gets fontified using lsp-face-semhl-comment); searching for the regexp \s< doesn't turn up anything either. Any hints for me on how I might verify that the feature is actually working? I'll add some preliminary comments to the review section now

sebastiansturm avatar Aug 28 '22 19:08 sebastiansturm

Thanks for the review! I'll address the code reviews next. As for testing the code, in my local tests, comment-search forward does find the "disabled" lines. Also paren/bracelet matchings are ignoring them. I'm using the provide snippet in this MR to test. Could you let me know what code you used for testing so I can reproduce? Thanks!

JimDBh avatar Aug 29 '22 16:08 JimDBh

this is what I'm using (with clangd, lsp-semantic-tokens-set-comment-syntax set to t):

int main() {
  int x = 0;
#ifdef NOT_DEFINED
  int y = 1;
#endif
  return x;
}

lsp-semantic-tokens--put-comment-syntax gets called with the right begin/end arguments, no luck with comment-search-forward though

sebastiansturm avatar Aug 29 '22 20:08 sebastiansturm

Thanks! However I was able to do comment-search-forward from the start of the buffer, and it ends up at the "i" character in the #ifdef macro (not sure why it wouldn't just be at the "#" sign, need to look into this a bit more)

JimDBh avatar Aug 29 '22 21:08 JimDBh

Upon further testing with a very large c-mode file with MANY such marcos, this patch makes lsp-mode lag a lot. The culprit seems to be text-property-search-forward/backward. I will see if we can keep a buffer-local cache of the added comment start/end pairs, and use that in the functions. In this way it should be a faster hopefully, and we also don't have to rely on text-property-search-forward, which is not available for 26.x.

JimDBh avatar Aug 30 '22 17:08 JimDBh

In this way it should be a faster hopefully, and we also don't have to rely on text-property-search-forward, which is not available for 26.x.

I think we can go ahead and drop 26.x at this point. Not sure if this helps solving your issue.

yyoncho avatar Nov 21 '22 18:11 yyoncho