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

libctags.a and mini-geany based on #2085 #2088

Merged
merged 10 commits into from May 13, 2019

Conversation

masatake
Copy link
Member

This is bease on @techee's work.
With this change, both ctags and mini-geany can be built.
This PR is just for showing the progress of my work.

@masatake masatake force-pushed the more-mini_geany branch 5 times, most recently from 20146d8 to 829c868 Compare April 29, 2019 09:54
main/parse.c Outdated Show resolved Hide resolved
@@ -333,13 +342,12 @@ extern int main (int argc, char **argv)
{
printf("\nParsing %s:\n", argv[i]);
/* createTags() is called repeatadly during Geany execution */
createTags(NULL, 0, argv[i], getNamedLanguage("C", 0));
tagArray = createTags(NULL, 0, argv[i], getNamedLanguage("C", 0));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting rid of using the static global variable is definitely nice. However, while this concrete approach works for this example, it isn't the best interface for Geany. When we call createTags(), we already have the tag array allocated inside TMSourceFile and we need to get the existing array filled and not newly created inside preWriteEntry() and then returned by createTags().

However, I think we already have all we need. We can just call

customWriter->private = our_tag_array; //or whatever we want
createTags(NULL, 0, argv[i], getNamedLanguage("C", 0));

and pass our custom data in and out this way without any return value of createTags(). The 'result' can be removed from postWriteEntry() too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
However, providing the way to return results from a custom writer is a good idea.
Though, this time is not generalizing, I would like to introduce this chanage of ctags side separately.
I will make another pull request for this change.

I will not change mini-geany side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, providing the way to return results from a custom writer is a good idea.

I'm not sure if it will ever be useful for anything since private can hold arbitrary user data (e.g. a struct, in Geany we'll pass TMSourceFile using it, see https://github.com/geany/geany/blob/master/src/tagmanager/tm_source_file.h#L32). Do you have any example when this might be needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The private field is designed as the place store data having rather short life type.
It is initialized with preWriteEntry method called from parseFileWithMio.
and set to NULL in postWriteEntry method called from parseFileWithMio.
This is designed for etags writer.
See postWriteEntry method of etags writer. It tells you much more than my English.

extern void writerSetup (MIO *mio)
{
	if (writer->preWriteEntry)
		writer->private = writer->preWriteEntry (writer, mio);
	else
		writer->private = NULL;
}

extern bool writerTeardown (MIO *mio, const char *filename)
{
	if (writer->postWriteEntry)
	{
		bool r;
		r = writer->postWriteEntry (writer, mio, filename);
		writer->private = NULL;
		return r;
	}
	return false;
}

Of course, you can bypass the life-cycle management done by ctags.
However, it is a kind of abuse.

Another approach is OO technique:

struct myCustomWriter {
   TagWriter base;
    extraLongLifeCycleData data;
   ...
};

This will be a good place to store TMSourceFile.

However, providing the way to return "the result of parsing a file" from ctags can make a client code a bit simple.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about passing custom data into createTags() rather than the data being returned by it - returning data is just a subset of what we may want to do and when we allow users to pass an arbitrary pointer inside, they have the flexibility to do about anything with the passed data. This is a pretty common pattern in various libraries, e.g.

https://developer.gnome.org/glib/stable/glib-Arrays.html#g-array-sort-with-data

where the user data is passed to the compare_func. In a similar way, createTags() could look like createTags(..., void *user_data) and it would store the user data somewhere, e.g. the extraLongLifeCycleData which would then be accessible from the writer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createTags(..., void *user_data)

Oh, I see. I will update our changes here. Thank you.

Copy link
Contributor

@techee techee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically this whole patch can be dropped since we can already pass all the data we need using writer->private.


return rval;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remove parseRawBuffer() from ctags completely and just export parseMio() which the client app would use directly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means we have to export mio API from ctags.
In the future, it is a good idea. However, in short term, it is not a good idea because, as I wrote, I introduced changes to mio ctags locally. So I think you cannot pass a mio instance in Geany to ctags.
As a temporary solution, we have to do
(1) rename mio in ctags to cmio or something, and
(2) export parseRawBuffer as non-mio API.

Here, I would like to introduce parseRawBuffer as the first step.
Is this acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, totally. I just suggested it as an option and maybe exposing MIO really isn't a good idea at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure exposing MIO makes much sense, it's only useful if the caller would actually do something we don't with MIO. That could include using MIO to create the buffer to be parsed (e.g. in memory), or creating it with a constructor we don't expose (e.g. mio_new_fp() in combination with fdopen()), but that's about it.

@masatake
Copy link
Member Author

I've tried to remove ctags_main. However, it seems that I should keep it as it was.
I will make one more FYI PR.

masatake and others added 8 commits May 11, 2019 04:04
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
… file

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
This is a part of commits for introducing libctags.a.
To add main.o to libctags.a, main.c should not have main()
function. As the temporary solution, this commit changes the
name of main() to ctags_cli_main().

This commit introduces cmd.c which will not be part of libctags.a.
The real main() function is put to cmd.c.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Suggested by techee.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
@masatake masatake force-pushed the more-mini_geany branch 5 times, most recently from f4ca5f6 to 223c0a5 Compare May 11, 2019 03:46
…es in ctags

This patch demonstrates how we use ctags in Geany as a library. The
patch tries to be minimalistic to demonstrate only the things we
need in ctags so it just directly replaces the ctags code with what
we need and the generated "ctags" binary is the demo app. For this
to be mergeable to ctags, there have to be some ifdefs specifying
whether ctags should be compiled as a library and mini-geany has
to be a separate binary.

In addition, @masatake did:
* fix a memleak about mio allocation.
* build both ctags and mini-geany
* include writer_p.h in parser.c (for writerRescanFailed)
* unify setTagWriter and setCustomTagWriter
* ...
@masatake masatake force-pushed the more-mini_geany branch 4 times, most recently from 09cf609 to c3245b2 Compare May 11, 2019 07:50
@coveralls
Copy link

coveralls commented May 11, 2019

Coverage Status

Coverage increased (+0.1%) to 85.483% when pulling 75c26bd on masatake:more-mini_geany into 9b28d8c on universal-ctags:master.

@masatake masatake force-pushed the more-mini_geany branch 10 times, most recently from c93f262 to 00e2e20 Compare May 12, 2019 12:12
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
@masatake masatake changed the title [FYI] studying #2085 libctags.a and mini-geany based on #2085 May 12, 2019
@masatake
Copy link
Member Author

@techee, I refect the most of all your suggestions.
Can I ask you to review this again?

@techee
Copy link
Contributor

techee commented May 13, 2019

@masatake I just went through the patches and tested it on my machine and everything looks pretty great! This really is a huge step towards having Geany and ctags synced - I'm really excited about this.

@masatake
Copy link
Member Author

Yes, this really important step. Thank you very much for you and Geany people's efforts.

The next TODOs are
(1) introduce "libctags.h" (or "ctags-api.h"), and
(2) move code in main.c to cmd.c as much as possible.

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.

None yet

4 participants