-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix(cpp): Allow declaring multiple functions and parenthetical initializers #3155
Conversation
Showing output from the developer tool visual mode is always nice for stuff like this, but I don't think this is what we want. The first item isn't a function and the second two definitely aren't... I was thinking we'd add Is there a reason you went this way instead? |
I tried adding to extern void f(int), g(char); (unless you don't think of that as a function declaration?). It also highlights the parens-as-initializer example better in a visual sense, though you're right that those aren't function declarations so that actual tags are incorrect. Happy to switch to |
OH... all this time I was thinking of variables... can you actually declare function declarations that way? I didn't know that. Declaration vs definition, correct? Is this valid syntax in C as well or only C++? |
Function isn't really intended to do variable declarations at all, that's just a side-effect of being impossible to distinguish so def a problem for another day. |
Yes, these are function prototypes, which are valid in C and C++. https://en.wikipedia.org/wiki/Function_prototype has some examples. int f(int);
int f(int x);
extern int f(int); It hadn't occurred to me before that C++ has a pretty ambiguous distinction between function prototypes and call-style (parenthetical) initializers: string f(string); // function prototype because string is a type
string s(x); // variable declaration because x is a variable So I don't think highlight.js should aim to distinguish between these; it should probably treat both as variable declarations. (And honestly a function prototype is essentially a variable declaration.) Currently we're calling both of them function definitions. I think the "right" answer is to only trigger function definition mode when there's a |
Actually we call it FUNCTION_DECLARATION as the idea is to capture both definitions and declarations with the rule. So I think this is a reasonable fix, but I think we should do it with a rule instead of "hiding" it inside a negative illegal match. Could you also apply this to the C grammar while we're fixing it for C++? |
Another question is does the context really matter... if we slapped everything like |
If I understand your last comment appropriately, you mean the following:
A separate declaration mode is needed so that a lone I agree that this should properly handle cases like I take it there isn't a goal to annotate variable declarations? Or should |
This is all changing (see the docs in Github). I need to see fix why they aren't getting published to readthedocs any longer.
Actually they aren't, they are tagged "function.dispatch" which is aliased back to
How would we if they are indistinguishable? Long term I'd like to see if the entire |
@@ -330,7 +330,7 @@ export default function(hljs) { | |||
NUMBERS | |||
] | |||
}, | |||
// allow for multiple declarations, ie: | |||
// allow for multiple declarations, e.g.: |
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.
LOL.
Changelog entry? |
I've ported the changes over to C, and now added to CHANGES, so perhaps this is ready to merge. I also realized I forgot to I can play with a second PR that aims to handle all the cases. I think we can get rid of the Thanks for clarifying all my confusion about the new intended labels, which indeed seem much nicer. I was looking at |
e75a8bd
to
a175948
Compare
Well mostly I'm trying to remove huge swaths of complexity when possible - with the idea that grammars should pattern match more than they should parse... If we remove the distinction then I'm pretty sure that I do think that at the least things could be simplified a lot with the new multi-scope tools, but not sure how familiar you are with those. |
I do wonder if this couldn't be built in the function regex detection itself... |
I'm not sure that I am. Could you recommend something to read? Is there a good language that's been fully modernized and would serve as a good example? (Most still seem to use |
That's just a name change really... all the new stuff is documented in the mode reference docs. |
You could look at the recent commit history as I'm not sure the new key names are used anywhere yet... |
@@ -0,0 +1,2 @@ | |||
<span class="hljs-function">string <span class="hljs-title">a</span><span class="hljs-params">(<span class="hljs-string">"hello"</span>)</span>, <span class="hljs-title">b</span><span class="hljs-params">(<span class="hljs-string">"world"</span>)</span></span>; | |||
<span class="hljs-function">vector<<span class="hljs-keyword">int</span>> <span class="hljs-title">u</span><span class="hljs-params">(<span class="hljs-number">1</span>)</span>, <span class="hljs-title">v</span><span class="hljs-params">(<span class="hljs-number">2</span>)</span></span>; |
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 you think it is declaring four functions? To me I think it's initializing four variables.
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.
If you have found a potential problem please open a new issue rather than commenting on a 3 year old issue.
Fix #3148 by allowing commas within a function declaration, to allow declaring multiple functions, as well as multiple parenthetical initializers.
Unfortunately, syntax like
(i.e. a mixture of parenthetical and equals-sign initializers) still isn't parsed correctly, because equals sign currently exits function mode. But this is progress at least.
Checklist
CHANGES.md
(should this go under## Unreleased
or similar section?)