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

lexer: c: Highlight #includes as Comment::PreprocFile #1770

Merged
merged 1 commit into from
Dec 27, 2021

Conversation

cdown
Copy link
Contributor

@cdown cdown commented Dec 1, 2021

When changing https://chrisdown.name over from Pygments to Rouge, one
quite noticeable change was that {% highlight c %} blocks lost the
delineation between #include statements and the actual include, with
them all now just being stylised as a comment.

This patch brings back Comment::PreprocFile support for C-like
languages, as it was with Pygments.

Tested locally by rebuilding chrisdown.name with a gem from this commit,
and seeing that the Comment::PreprocFile highlighting reappeared.

@cdown
Copy link
Contributor Author

cdown commented Dec 1, 2021

Example change as seen on https://chrisdown.name/2020/01/13/1195725856-and-friends-the-origins-of-mysterious-numbers.html

Pygments:

2021-12-01_135426_599100737

Rouge HEAD:

2021-12-01_135541_105729703

Rouge after this PR:

2021-12-01_135605_112637722

@tancnle tancnle added the needs-review The PR needs to be reviewed label Dec 10, 2021
@cdown
Copy link
Contributor Author

cdown commented Dec 19, 2021

Hey folks, just checking if you have any thoughts on this? Right now I'm running a patched build of Rouge, but especially since you folks just did a release it would be great to get back on mainline if possible :-)

Tagging some folks from PRs I see reviewed recently in the hope that someone has an opinion, sorry to bother. @tancnle @dblessing

@tancnle
Copy link
Collaborator

tancnle commented Dec 27, 2021

@cdown Thank you for your contribution ❤️ The change looks good overall. I have a look at the current support from Pygments and wonder if we could reuse the vetted pattern. Pygments seems to support the following patterns.

#include <string.h> /* this is a comment */
#include <string.h> // this is a comment

# include <string.h> 
#/* this is a comment */ include <string.h> 
#include  /* this is a comment */  <string.h> 

For this PR, I would suggest at least supporting the top 2 with trailing comments. The change in this PR will display comments as errors so we might need to adjust it slighly.

Screen Shot 2021-12-27 at 3 12 41 pm

Implementation wise, I'd suggest

  1. Use include mixin in the macro state.
diff --git lib/rouge/lexers/c.rb lib/rouge/lexers/c.rb
index 4df87a4f..ad6624a2 100644
--- lib/rouge/lexers/c.rb
+++ lib/rouge/lexers/c.rb
@@ -66,7 +66,6 @@ module Rouge
         mixin :inline_whitespace

         rule %r/#if\s0/, Comment, :if_0
-        rule %r/#include\s*/, Comment::Preproc, :include
         rule %r/#/, Comment::Preproc, :macro

         rule(//) { pop! }
@@ -171,18 +170,22 @@ module Rouge
       end

       state :macro do
-        # NB: pop! goes back to :bol
-        rule %r/\n/, Comment::Preproc, :pop!
+        mixin :include
         rule %r([^/\n\\]+), Comment::Preproc
         rule %r/\\./m, Comment::Preproc
         mixin :inline_whitespace
         rule %r(/), Comment::Preproc
+        # NB: pop! goes back to :bol
+        rule %r/\n/, Comment::Preproc, :pop!
       end

       state :include do
-        rule %r/\n/, Comment::PreprocFile, :pop!
-        rule %r/<[^\n>]+>/, Comment::PreprocFile
-        rule %r/"[^\n"]+"/, Comment::PreprocFile
+        rule %r/(include)(\s*)(<[^>]+>)([^\n]*)/ do
+          groups Comment::Preproc, Text, Comment::PreprocFile, Comment::Single
+        end
+        rule %r/(include)(\s*)("[^"]+")([^\n]*)/ do
+          groups Comment::Preproc, Text, Comment::PreprocFile, Comment::Single
+        end
       end

       state :if_0 do
  1. Add examples with trailing comments in spec/visual/samples/c
#include <string.h> /* this is a comment */
#include <string.h> // this is a comment

I am not a C programmer and would appreciate any feedback 🙏🏼

@tancnle tancnle added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Dec 27, 2021
@tancnle tancnle self-requested a review December 27, 2021 04:18
@tancnle tancnle mentioned this pull request Dec 27, 2021
When changing https://chrisdown.name over from Pygments to Rouge, one
quite noticeable change was that `{% highlight c %}` blocks lost the
delineation between #include statements and the actual include, with
them all now just being stylised as a comment.

This patch brings back Comment::PreprocFile support for C-like
languages, as it was with Pygments.

Tested locally by rebuilding chrisdown.name with a gem from this commit,
and seeing that the Comment::PreprocFile highlighting reappeared.
@cdown cdown force-pushed the cdown/2021-12-01/preprocfile branch from c485dec to 4d09736 Compare December 27, 2021 12:50
@cdown
Copy link
Contributor Author

cdown commented Dec 27, 2021

@tancnle Thank you for the kind review and detailed suggestions! I've made the requested changes, and have also added and visually verified a couple of other valid parsing cases to spec/visual/samples/c.

@tancnle tancnle merged commit 1db8d15 into rouge-ruby:master Dec 27, 2021
@tancnle
Copy link
Collaborator

tancnle commented Dec 27, 2021

Thank you for your prompt response @cdown 👍🏼

@cdown
Copy link
Contributor Author

cdown commented Dec 27, 2021

You too!

@tancnle tancnle removed the author-action The PR has been reviewed but action by the author is needed label Jun 14, 2022
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.

2 participants