<p><b>@b4n</b> requested changes on this pull request.</p>
<p>Comments on the uctags/geany diff:</p>
<div class="highlight highlight-source-diff"><pre><span class="pl-mi1">+static bool createTagsWithFallback1 (const langType language,</span>
<span class="pl-mi1">+ passStartCallback passCallback, void *userData)</span>
<span class="pl-mi1">+{</span>
<span class="pl-mi1">+ int lastPromise = getLastPromise ();</span>
<span class="pl-mi1">+ unsigned int passCount = 0;</span>
<span class="pl-mi1">+ rescanReason whyRescan;</span>
<span class="pl-mi1">+</span>
<span class="pl-mi1">+ if (LanguageTable [language]->useCork)</span>
<span class="pl-mi1">+ corkTagFile();</span></pre></div>
<p>Doesn't this miss <code>initializeParser (language);</code>?</p>
<hr>
<p>In <code>runParserInNarrowedInputStream()</code>: shouldn't more be guarded? Here the meat of the function is disabled with <code>CTAGS_LIB</code>, but not all.</p>
<hr>
<div class="highlight highlight-source-diff"><pre><span class="pl-md">-static void attachEndFieldMaybe (int macroCorkIndex)</span>
<span class="pl-md">-{</span>
<span class="pl-md">- if (macroCorkIndex != CORK_NIL)</span>
<span class="pl-md">- {</span>
<span class="pl-md">- tagEntryInfo *tag;</span>
<span class="pl-md">-</span>
<span class="pl-md">- tag = getEntryInCorkQueue (macroCorkIndex);</span>
<span class="pl-md">- tag->extensionFields.endLine = getInputLineNumber ();</span>
<span class="pl-md">- }</span>
<span class="pl-md">-}</span>
<span class="pl-md">-</span></pre></div>
<p>Would be nice to have that :)</p>
<hr>
<p>In <code>directiveDefine()</code>: Why use a completely different signature collection logic than CTags'? I get that might be related to c.c's handling of it, but not sure how what affects what.</p><hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">configure.ac</a>:</p>
<pre style='color:#555'>> @@ -38,7 +38,14 @@ AC_PROG_LN_S
# autoscan start
# Checks for header files.
-AC_CHECK_HEADERS([fcntl.h fnmatch.h glob.h stdlib.h sys/time.h])
+AC_CHECK_HEADERS([fcntl.h glob.h stdlib.h sys/time.h errno.h limits.h])
+
+# Checks for dependencies needed by ctags
+AC_CHECK_HEADERS([fnmatch.h direct.h io.h sys/dir.h])
+AH_TEMPLATE([USE_STDBOOL_H], [whether or not to use <stdbool.h>.])
+AC_DEFINE([USE_STDBOOL_H])
+AH_TEMPLATE([CTAGS_LIB], [compile ctags as a library.])
+AC_DEFINE([CTAGS_LIB])
</pre>
<p>Should rather use the <code>AC_DEFINE(symbol, value, description)</code> than calling <code>AH_TEMPLATE</code> manually IMO:</p>
<pre><code class="language-autoconf">AC_DEFINE([USE_STDBOOL_H], [1], [whether or not to use <stdbool.h>.])
AC_DEFINE([CTAGS_LIB], [1], [compile ctags as a library.])
</code></pre>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/asciidoc.c</a>:</p>
<pre style='color:#555'>>
while (1)
{
nl = nestingLevelsGetCurrent(nestingLevels);
- if (nl && nl->type >= kind)
+ e = getEntryOfNestingLevel (nl);
+ if ((nl && (e == NULL)) || (e && (e->kind - AsciidocKinds) >= kind))
</pre>
<p>why do that when <code>e == NULL</code>?</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/asciidoc.c</a>:</p>
<pre style='color:#555'>> {
- e.extensionFields.scopeKind = &(AsciidocKinds [nl->type]);
- e.extensionFields.scopeName = vStringValue (nl->name);
+ e.extensionFields.scopeKind = &(AsciidocKinds [parent->kind - AsciidocKinds]);
+ e.extensionFields.scopeName = parent->name;
</pre>
<p>isn't the whole scope part dealt with by Cork anyway?</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/asciidoc.c</a>:</p>
<pre style='color:#555'>> {
- e.extensionFields.scopeKind = &(AsciidocKinds [nl->type]);
- e.extensionFields.scopeName = vStringValue (nl->name);
+ e.extensionFields.scopeKind = &(AsciidocKinds [parent->kind - AsciidocKinds]);
</pre>
<p>seems like a convoluted way to say <code>parent->kind</code> :)</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/c.c</a>:</p>
<pre style='color:#555'>> @@ -290,12 +289,12 @@ static kindOption CKinds [] = {
{ true, 'g', "enum", "enumeration names"},
{ true, 'm', "member", "class, struct, and union members"},
{ true, 'n', "namespace", "namespaces"},
- { false, 'p', "prototype", "function prototypes"},
+ { true, 'p', "prototype", "function prototypes"},
</pre>
<p>that's odd… but I would have imagine those would have been enabled before. Weird.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/c.c</a>:</p>
<pre style='color:#555'>> @@ -1221,11 +1219,10 @@ static void addOtherFields (tagEntryInfo* const tag, const tagType type,
{
tag->extensionFields.access = accessField (st);
}
- if ((true == st->gotArgs) && (true == Option.extensionFields.argList) &&
</pre>
<p>is that option gone? Not that we actually do need it, but well</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/c.c</a>:</p>
<pre style='color:#555'>>
contextual_fake_count = 0;
Assert (passCount < 3);
- cppInit ((bool) (passCount > 1), isInputLanguage (Lang_csharp), isInputLanguage (Lang_cpp), &(CKinds [CK_DEFINE]));
+
+ kind_for_define = CKinds+CK_DEFINE;
+
+ cppInit ((bool) (passCount > 1), isInputLanguage (Lang_csharp), isInputLanguage(Lang_cpp),
+ kind_for_define);
</pre>
<p>why introduce the <code>kind_for_define</code> variable?</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/c.c</a>:</p>
<pre style='color:#555'>>
exception = (exception_t) setjmp (Exception);
- retry = false;
</pre>
<p>shouldn't <code>rescan</code> be reset to <code>RESCAN_NONE</code> here? <code>setjmp()</code> might lead to jumping there directly.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/python.c</a>:</p>
<pre style='color:#555'>> vStringCatS(result, ".");
else
vStringCatS(result, "/");
*/
}
- vStringCat(result, nl->name);
- is_class = (nl->type == K_CLASS);
+
+ e = getEntryOfNestingLevel (nl);
+ if (e)
+ {
+ vStringCatS(result, e->name);
+ is_class = ((e->kind - PythonKinds) == K_CLASS);
+ }
+ else
+ is_class = K_FUNCTION; /* ??? */
</pre>
<p>shouldn't that be merely <code>false</code>?</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/python.c</a>:</p>
<pre style='color:#555'>> prev = nl;
}
return is_class;
}
/* Check indentation level and truncate nesting levels accordingly */
-static void checkIndent(NestingLevels *nls, int indent)
+static void checkIndent(NestingLevels *nls, int indent, bool eof)
</pre>
<p>unused last (new) param</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/python.c</a>:</p>
<pre style='color:#555'>> @@ -27,6 +27,17 @@
/*
* DATA DEFINITIONS
*/
+
+struct corkPair {
+ int index;
+};
</pre>
<p>that struct looks useless.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/python.c</a>:</p>
<pre style='color:#555'>> @@ -825,5 +859,6 @@ extern parserDefinition *PythonParser (void)
def->kindCount = ARRAY_SIZE (PythonKinds);
def->extensions = extensions;
def->parser = findPythonTags;
+ def->useCork = true;
return def;
}
</pre>
<p>well, I guess all changes to <em>python.c</em> are not too relevant so long as the parser still works, as I plan to import my new rewritten parser prom UCtags once Cork is available on our side.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/rest.c</a>:</p>
<pre style='color:#555'>>
while (1)
{
nl = nestingLevelsGetCurrent(nestingLevels);
- if (nl && nl->type >= kind)
+ e = getEntryOfNestingLevel (nl);
+ if ((nl && (e == NULL)) || (e && (e->kind - RestKinds) >= kind))
</pre>
<p>same question as why handling <code>e == NULL</code> on the left</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/parsers/rest.c</a>:</p>
<pre style='color:#555'>> {
- e.extensionFields.scopeKind = &(RestKinds [nl->type]);
- e.extensionFields.scopeName = vStringValue (nl->name);
+ e.extensionFields.scopeKind = &(RestKinds [parent->kind - RestKinds]);
+ e.extensionFields.scopeName = parent->name;
</pre>
<p>again, same as for asciidoc</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">src/tagmanager/tm_source_file.c</a>:</p>
<pre style='color:#555'>> - {
- GStatBuf s;
-
- /* load file to memory and parse it from memory unless the file is too big */
- if (g_stat(file_name, &s) != 0 || s.st_size > 10*1024*1024)
- parse_file = TRUE;
- else
- {
- if (!g_file_get_contents(file_name, (gchar**)&text_buf, (gsize*)&buf_size, NULL))
- {
- g_warning("Unable to open %s", file_name);
- return FALSE;
- }
- free_buf = TRUE;
- }
- }
</pre>
<p>shouldn't we keep this?</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">tests/ctags/bug507864.c.tags</a>:</p>
<pre style='color:#555'>> @@ -1,4 +1,5 @@
# format=tagmanager
-ENTSEQNO�16�(seq)�0�FUNCSTS
-MEMTXT�16�(form_msg)�0�
-MEMTXT�1024�(form_msg)�0�
+ENTSEQNO�16�(eq)�0�FUNCSTS
</pre>
<p>should really be <code>(seq)</code> if anything</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">tests/ctags/bug507864.c.tags</a>:</p>
<pre style='color:#555'>> @@ -1,4 +1,5 @@
# format=tagmanager
-ENTSEQNO�16�(seq)�0�FUNCSTS
-MEMTXT�16�(form_msg)�0�
-MEMTXT�1024�(form_msg)�0�
+ENTSEQNO�16�(eq)�0�FUNCSTS
+MEMTXT�16�(mail)�0�
+MEMTXT�1024�(orm_msg)�0�FUNCSTS
</pre>
<p>here too, should be <code>(from_msg)</code></p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">tests/ctags/simple.d.tags</a>:</p>
<pre style='color:#555'>> @@ -20,5 +20,5 @@ obj
qar�64�Struct.Union�0�int
quxx�64�Struct.Union�0�bool
test.simple�256�0
-tfun�16�(T)�Class�0�auto
+tfun�16�(T v)�Class�0�auto
</pre>
<p>not sure if it's better or worse. Source line is</p>
<div class="highlight highlight-source-d"><pre> <span class="pl-k">auto</span> <span class="pl-en">tfun</span>(T)(T v)
{
<span class="pl-k">return</span> v;
}</pre></div>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/main/entry.c</a>:</p>
<pre style='color:#555'>> @@ -114,6 +150,14 @@ extern const char *tagFileName (void)
* Pseudo tag support
*/
+extern void abort_if_ferror(MIO *const mio)
+{
+#ifndef CTAGS_LIB
+ if (mio_error (mio))
+ error (FATAL | PERROR, "cannot write tag file");
+#endif
</pre>
<p>It maybe worth still doing <em>something</em>? warn maybe?</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/main/entry.c</a>:</p>
<pre style='color:#555'>> + if (includeExtensionFlags ()
+ && isXtagEnabled (XTAG_QUALIFIED_TAGS)
+ && doesInputLanguageRequestAutomaticFQTag ())
+ buildFqTagCache (tag);
+
+#ifdef CTAGS_LIB
+ getTagScopeInformation((tagEntryInfo *)tag, NULL, NULL);
+
+ if (TagEntryFunction != NULL)
+ {
+ ctagsTag t;
+
+ initCtagsTag(&t, tag);
+ length = TagEntryFunction(&t, TagEntryUserData);
+ }
+#else
</pre>
<p>Couldn't we simply have our own writer? Might be more work, but maybe less divergence?</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/main/lcpp.c</a>:</p>
<pre style='color:#555'>> return NULL;
+
+ start = strdup (vStringValue (signature));
</pre>
<p>not standard, maybe use <code>eStrdup()</code>?</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080">ctags/main/options.h</a>:</p>
<pre style='color:#555'>> extern bool isExcludedFile (const char* const name);
+extern bool isIncludeFile (const char *const fileName);
+/* GEANY DIFF */
+/* extern const ignoredTokenInfo * isIgnoreToken (const char *const name); */
extern bool isIgnoreToken (const char *const name, bool *const pIgnoreParens, const char **const replacement);
</pre>
<p>why not use the new ctags impl? I mean, it seems to do the same, but differently, couldn't we use that?</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-10056080">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABDrJ5Ye2DVGWAkeHNzKdANWCgnQpF7gks5rBbWggaJpZM4KXvBh">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/ABDrJyqTSznFCwk7T0JoD9DUQ9UsEuQ3ks5rBbWggaJpZM4KXvBh.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="https://github.com/geany/geany/pull/1263#pullrequestreview-10056080"></link>
<meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>
<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://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.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-10056080"}}}</script>