<p><b>@b4n</b> requested changes on this pull request.</p>
<p>OK, here's my latest with the help of GCC.<br>
See also <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="389002355" data-permission-text="Issue title is private" data-url="https://github.com/techee/geany/issues/1" data-hovercard-type="pull_request" data-hovercard-url="/techee/geany/pull/1/hovercard" href="https://github.com/techee/geany/pull/1">techee#1</a> which contains some small stuff that I think are relevant, but please review them, don't take them as gospel.</p>
<p>Apart from that, I think the next thing is merging an see how it goes, as as far as I can tell it works very well.</p><hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#discussion_r236970115">ctags/main/parse.c</a>:</p>
<pre style='color:#555'>> pushNarrowedInputStream (language,
startLine, startCharOffset,
endLine, endCharOffset,
sourceLineOffset);
#ifndef CTAGS_LIB
tagFileResized = createTagsWithFallback1 (language);
+#else
+ /* Simple parsing without rescans - not used by any sub-parsers anyway */
</pre>
<p>I don't really care right now because as you pointed out there's no use case yet, but you mentioning in the commit message that "the C and Fortran parsers" being the only parsers using retry mode and that they do not support sub-parsers makes the retry case irrelevant is not correct: nothing prevents a parser from calling the C or Fortran parsers as sub-parsers. IIUC, the upstream Flex or so parser actually does this.</p>
<p>But as said, don't worry too much about that right now; we can fix this stuff when we actually have use for it.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#discussion_r240014456">ctags/main/lcpp.c</a>:</p>
<pre style='color:#555'>> else if (c == SINGLE_QUOTE)
break;
else if (c == NEWLINE)
{
- ungetcToInputFile (c);
- break;
- }
- else if (count == 1 && strchr ("DHOB", toupper (c)) != NULL)
- veraBase = c;
- else if (veraBase != '\0' && ! isalnum (c))
- {
</pre>
<p>I can't find these lines being suppressed upstream, so I assume it's a local change. However, you should then remove the variable declaration for <code>veraBase</code> as well.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#discussion_r240014806">ctags/parsers/python.c</a>:</p>
<pre style='color:#555'>> vString *const parent, int is_class_parent, const char *arglist)
{
tagEntryInfo tag;
+ int corkIndex;
+ int fqCorkIndex = CORK_NIL;
+ const struct corkInfo nilInfo = {CORK_NIL};
</pre>
<p>unused variables <code>fqCorkIndex</code> and <code>nilInfo</code>, any reason to have them?</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#discussion_r240014878">ctags/parsers/python.c</a>:</p>
<pre style='color:#555'>> vString *const parent, int is_class_parent, const char *arglist)
{
tagEntryInfo tag;
+ int corkIndex;
+ int fqCorkIndex = CORK_NIL;
+ const struct corkInfo nilInfo = {CORK_NIL};
</pre>
<p>I see they were upstream in the old Python parser, but they were used at the time.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#discussion_r240014936">ctags/parsers/rest.c</a>:</p>
<pre style='color:#555'>>
if (vStringLength (name) > 0)
{
+ tagEntryInfo *parent = getEntryOfNestingLevel (nl);
</pre>
<p>This variable is unused, but upstream (<em>rst.c</em>) uses it to mess with the scope (which seems stupid, but well). Unused variables have that issue that people have a tendency to remove them… which generally seems legitimate.<br>
BTW, if we remove this, then <code>nl</code> gets unused, but we still gotta call <code>getNestingLevel()</code> because it does the <code>nestingLevelsPop()</code> stuff needed.<br>
What do you think?</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#discussion_r240017919">ctags/parsers/asciidoc.c</a>:</p>
<pre style='color:#555'>>
if (vStringLength (name) > 0)
{
+ tagEntryInfo *parent = getEntryOfNestingLevel (nl);
</pre>
<p>same here than in <em>rest.c</em>, <code>parent</code> is unused, same question applies.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-179157159">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABDrJw-0bUKDxt9PAkAbLSY9m1gF6h9Pks5u3NR3gaJpZM4KXvBh">mute the thread</a>.<img src="https://github.com/notifications/beacon/ABDrJ05JNRObtVKCU7ZbCAEWMWE6Fcp9ks5u3NR3gaJpZM4KXvBh.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/geany/geany","title":"geany/geany","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/geany/geany"}},"updates":{"snippets":[{"icon":"PERSON","message":"@b4n requested changes on #1263"}],"action":{"name":"View Pull Request","url":"https://github.com/geany/geany/pull/1263#pullrequestreview-179157159"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/geany/geany/pull/1263#pullrequestreview-179157159",
"url": "https://github.com/geany/geany/pull/1263#pullrequestreview-179157159",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
},
{
"@type": "MessageCard",
"@context": "http://schema.org/extensions",
"hideOriginalBody": "false",
"originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB",
"title": "@b4n requested changes on 1263",
"sections": [
{
"text": "OK, here's my latest with the help of GCC.\r\nSee also https://github.com/techee/geany/pull/1 which contains some small stuff that I think are relevant, but please review them, don't take them as gospel.\r\n\r\nApart from that, I think the next thing is merging an see how it goes, as as far as I can tell it works very well.",
"activityTitle": "**Colomban Wendling**",
"activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png",
"activitySubtitle": "@b4n",
"facts": [
]
}
],
"potentialAction": [
{
"targets": [
{
"os": "default",
"uri": "https://github.com/geany/geany/pull/1263#pullrequestreview-179157159"
}
],
"@type": "OpenUri",
"name": "View on GitHub"
},
{
"name": "Unsubscribe",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 173994081\n}"
}
],
"themeColor": "26292E"
}
]</script>