-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
New language: CIL #1593
New language: CIL #1593
Conversation
Couple of quick notes:
|
Done - thanks for the comment, @mAAdhaTTah! I was wondering how that worked. Looks like you've got some vulnerabilities in your required
|
components/prism-cil.js
Outdated
@@ -0,0 +1,26 @@ | |||
Prism.languages.cil = { | |||
'displayTitle': "CIL", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
displayTitle
doesn't go here, you already have it in the components.json.
Thanks for the contribution. One minor comment, but I'll need someone with stronger regex skills to give this a full review. Also, if this is a .NET extension, should this extend from the |
CIL is a language on its own and C# is to CIL basically what C is to Assembly; Namely, C# compiles to CIL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thank you for this amazing PR.
But there are a few things that have to be done before we can merge this:
- Please remove the
package-lock.json
. - Please remove the small changes made to
prism.js
.
I left you a few comments and questions. Also, it'd be great if you were to add some tests.
components/prism-cil.js
Outdated
Prism.languages.cil = { | ||
'displayTitle': "CIL", | ||
|
||
'comment': /\/\/[^\n]*(?=\r?\n)/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because [^\n]*
is greedy it will also consume the \r
at the end of a line.
The pattern /\/\/.*/
will do what you want and is really simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, does CIL support multi-line comments?
components/prism-cil.js
Outdated
|
||
'class-name': { | ||
lookbehind: true, | ||
pattern: /([^a-zA-Z0-9])\.[a-z]+\b/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nitpicky but we usually have the pattern
before everything else (namely: lookbehind
).
components/prism-cil.js
Outdated
'number': /\b(0x)?[0-9a-fA-F]+\b/i, | ||
'punctuation': /[{}[\];(),:]|IL_[0-9A-Za-z]+/, | ||
|
||
'keyword': /\b(iant|currency|syschar|void|bool|int8|int16|int32|int64|float32|float64|error|unsigned int8|unsigned int16|unsigned int32|unsigned int64|nativeType|decimal|date|bstr|lpstr|lpwstr|lptstr|objectref|iunknown|idispatch|struct|interface|int|unsigned int|nested struct|byvalstr|ansi bstr|tbstr|variant bool|lpstruct|static|public|private|family|final|specialname|virtual|abstract|assembly|default|cil managed|famandassem|famorassem|privatescope|hidebysig|newslot|rtspecialname|unmanagedexp|reqsecobj|pinvokeimpl|value|enum|interface|sealed|abstract|auto|sequential|explicit|ansi|unicode|autochar|import|serializable|nested public|nested private|nested family|nested assembly|nested famandassem|nested famorassem|beforefieldinit|specialname|rtspecialname|class|instance|string|extern|extends)\b/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things about this keyword list:
initonly
is missing.- Please split things like
unsigned int32
intounsigned
andint32
. This will reduce the number of keywords. - Please sort the keywords alphabetically. This makes the list easier to maintain and you will notice the keywords which are included twice.
components/prism-cil.js
Outdated
|
||
'boolean': /\b(?:true|false)\b/, | ||
'number': /\b(0x)?[0-9a-fA-F]+\b/i, | ||
'punctuation': /[{}[\];(),:]|IL_[0-9A-Za-z]+/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
and =
should also be punctuation.
components/prism-cil.js
Outdated
'variable': /\[\w+\]/, | ||
|
||
'boolean': /\b(?:true|false)\b/, | ||
'number': /\b(0x)?[0-9a-fA-F]+\b/i, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about floating point numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I'll update now.
components/prism-cil.js
Outdated
|
||
'keyword': /\b(iant|currency|syschar|void|bool|int8|int16|int32|int64|float32|float64|error|unsigned int8|unsigned int16|unsigned int32|unsigned int64|nativeType|decimal|date|bstr|lpstr|lpwstr|lptstr|objectref|iunknown|idispatch|struct|interface|int|unsigned int|nested struct|byvalstr|ansi bstr|tbstr|variant bool|lpstruct|static|public|private|family|final|specialname|virtual|abstract|assembly|default|cil managed|famandassem|famorassem|privatescope|hidebysig|newslot|rtspecialname|unmanagedexp|reqsecobj|pinvokeimpl|value|enum|interface|sealed|abstract|auto|sequential|explicit|ansi|unicode|autochar|import|serializable|nested public|nested private|nested family|nested assembly|nested famandassem|nested famorassem|beforefieldinit|specialname|rtspecialname|class|instance|string|extern|extends)\b/, | ||
|
||
'function': /\b(add|add\.ovf|add\.ovf\.un|and|arglist|beq|beq\.s|bge|bge\.s|bge\.un|bge\.un\.s|bgt|bgt\.s|bgt\.un|bgt\.un\.s|ble|ble\.s|ble\.un|ble\.un\.s|blt|blt\.s|blt\.un|blt\.un\.s|bne\.un|bne\.un\.s|box|br|br\.s|break|brfalse|brfalse\.s|brinst|brinst\.s|brnull|brnull\.s|brtrue|brtrue\.s|brzero|brzero\.s|call|calli|callvirt|castclass|ceq|cgt|cgt\.un|ckfinite|clt|clt\.un|constrained\.|conv\.i|conv\.i1|conv\.i2|conv\.i4|conv\.i8|conv\.ovf\.i|conv\.ovf\.i\.un|conv\.ovf\.i1|conv\.ovf\.i1\.un|conv\.ovf\.i2|conv\.ovf\.i2\.un|conv\.ovf\.i4|conv\.ovf\.i4\.un|conv\.ovf\.i8|conv\.ovf\.i8\.un|conv\.ovf\.u|conv\.ovf\.u\.un|conv\.ovf\.u1|conv\.ovf\.u1\.un|conv\.ovf\.u2|conv\.ovf\.u2\.un|conv\.ovf\.u4|conv\.ovf\.u4\.un|conv\.ovf\.u8|conv\.ovf\.u8\.un|conv\.r\.un|conv\.r4|conv\.r8|conv\.u|conv\.u1|conv\.u2|conv\.u4|conv\.u8|cpblk|cpobj|div|div\.un|dup|endfault|endfilter|endfinally|initblk|initobj|isinst|jmp|ldarg|ldarg\.0|ldarg\.1|ldarg\.2|ldarg\.3|ldarg\.s|ldarga|ldarga\.s|ldc\.i4|ldc\.i4\.[0-9]+|ldc\.i4\.m1|ldc\.i4\.M1|ldc\.i4\.s|ldc\.i8|ldc\.r4|ldc\.r8|ldelem|ldelem\.i|ldelem\.i1|ldelem\.i2|ldelem\.i4|ldelem\.i8|ldelem\.r4|ldelem\.r8|ldelem\.ref|ldelem\.u1|ldelem\.u2|ldelem\.u4|ldelem\.u8|ldelema|ldfld|ldflda|ldftn|ldind\.i|ldind\.i1|ldind\.i2|ldind\.i4|ldind\.i8|ldind\.r4|ldind\.r8|ldind\.ref|ldind\.u1|ldind\.u2|ldind\.u4|ldind\.u8|ldlen|ldloc|ldloc\.[0-9]+|ldloc\.s|ldloca|ldloca\.s|ldnull|ldobj|ldsfld|ldsflda|ldstr|ldtoken|ldvirtftn|leave|leave\.s|localloc|mkrefany|mul|mul\.ovf|mul\.ovf\.un|neg|newarr|newobj|nop|not|or|pop|readonly\.|refanytype|refanyval|rem|rem\.un|ret|rethrow|shl|shr|shr\.un|sizeof|starg|starg\.s|stelem|stelem\.i|stelem\.i1|stelem\.i2|stelem\.i4|stelem\.i8|stelem\.r4|stelem\.r8|stelem\.ref|stfld|stind\.i|stind\.i1|stind\.i2|stind\.i4|stind\.i8|stind\.r4|stind\.r8|stind\.ref|stloc|stloc\.0|stloc\.1|stloc\.2|stloc\.3|stloc\.s|stobj|stsfld|sub|sub\.ovf|sub\.ovf\.un|switch|tail\.|throw|unaligned\.|alignment|unbox|unbox\.any|volatile\.|xor)\b/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why do some end with
.
? - Please sort this list by descending length.
Because of the way alternations work and the fact that.
is not included in\w
(which is why\b
won't help), some function names are not going to be matched correctly.
E.g.:add.ovf
will be matched as[add].ovf
.
This is because alternations will use the first one that matches (in this caseadd
). - Please compress the list a little.
There is a lot of redundancy which can be used to make the list smaller.
E.g.: All functions which start withconv
can be matched with the following pattern:conv\.(?:[iu][1248]?|ovf\.[iu][1248]?(?:\.un)?|r\.un|r4|r8)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I originally pulled out of the Wikipedia article with a bit of JS and a regex or two to process it.
components.json
Outdated
@@ -57,6 +57,10 @@ | |||
}, | |||
"option": "default" | |||
}, | |||
"cil": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to place this further down below (under C++, I think).
These here are special languages for Prism.
All done! Is that better? |
Looks good! There are still a few things I noticed:
|
I've made the changes requested (even though it's recommended that you commit How do I add tests? Is there any documentation on this? |
Here's the documentation on how to add tests. |
Ah, thanks! Looking at it, I think test cases are going to be challenging as I don't know the language well enough to create a valid cut-down program that would do the job as a test case. In addition, there's a lot of boilerplate code that's appears to be common across multiple IL files, which further complicates the process of writing tests. Thoughts? |
Prism test cases usually test specific features (e.g. numbers, comments, small aspects of a language in general). So you don't have to come up with a minimal program which includes all features. Just write small tests which test one feature each. For examples on how to do tests right, I suggest looking at the JavaScript and C-like tests. |
While playing around I also found a few minor things:
|
Thanks for the comments! PR updated. This is a much longer process than I thought it would be :P |
@sbrl Looks good! I found another missing keyword :) Don't worry, this is going to be the last nit. |
Done, @RunDevelopment |
Looks good! Tests pass. @mAAdhaTTah I think we are ready to merge this. |
What's the status on this please, @RunDevelopment? I thought I'd done everything required, but it's been a while and this hasn't been merged yet? Did I miss something? |
@sbrl You've done everything we asked of and you definitely have my OK. So don't worry! This has not been forgotten. |
@RunDevelopment @sbrl I haven't forgotten! Holidays are busy :). I want to take a pass through Prism's PRs sometime this week(end) so I'll be taking a closer look at this soon. |
@sbrl If you can resolve the conflict (just merge up from |
Done, @mAAdhaTTah :D |
@sbrl Thank you for the contribution! |
Add support for the Common Intermediate Language from .NET.
I discovered that Prism.js doesn't have CIL (Common Intermediate Language, from .NET) support when I wrote a blog post that contained some - so I decided to create my own grammar, since the process looked fairly easy (it was!).