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

Server guessing #225

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Server guessing #225

wants to merge 2 commits into from

Conversation

Hylen
Copy link
Contributor

@Hylen Hylen commented Aug 10, 2015

This is just to give you an idea of in what direction I'm working, since I'd like to hear what you think before I put in more time. This is basically the caching of the compilation database, but it's a very crude solution so far. It also contains the reconstruction of the file names I was talking about.

Things I will change/continue to work on:

  • TODO Parse the whole database and save using C++ std::string instead?
  • TODO Reparse database if out of date
  • TODO Add command for forcing a reparse?
  • TODO Close paths guessing algorithm
  • TODO Menu for choosing the compile command
  • TODO Implement file path operations (get rid of Boost)
  • TODO Testing of the above

@Sarcasm
Copy link
Owner

Sarcasm commented Aug 10, 2015

This is basically the caching of the compilation database, but it's a very crude solution so far.

Good idea.

It also contains the reconstruction of the file names I was talking about.

I don't like this solution, I don't think one should rely on -c, that's not the good way to get the file. Also right now IIUC, the files aren't like to their compile commands, which is not that useful.


I think, you should work on caching alone first. Get this right and then think about the guessing.


TODO Parse the whole database and save using C++ std::string instead?

Don't understand.

TODO Reparse database if out of date

Yes, necessary for caching.

TODO Add command for forcing a reparse?

Are you expecting the caching to be incorrect? :)

TODO Close paths guessing algorithm

Another PR from caching.

TODO Menu for choosing the compile command

This may be useful, I haven't found the need yet though, one may want to sleep on this one and work other stuff until the need arise.

@Hylen
Copy link
Contributor Author

Hylen commented Aug 11, 2015

I don't like this solution, I don't think one should rely on -c, that's not the good way to get the file. Also right now IIUC, the files aren't like to their compile commands, which is not that useful.

In what way does CompilationDatabase::constructFileNames rely on -c? It simply takes the first compilation argument not prefixed by a dash and skips the output file if it is present. I'm aware this is probably not the perfect solution, and I will update the details when I have aquired data from different systems/projects as test cases.

The files are linked to their compilation commands through clang_CompilationDatabase_getCompileCommands. I assume the database is a map, in which case this should be fine.

More importantly, what do you think of the general approch? I think obtaining the file names from the compile commands is the right way to go, even if it needs more work than this.

I think, you should work on caching alone first. Get this right and then think about the guessing.

I'd rather have a proof of concept first, both for caching and guessing. When I see you agree on the direction my work is taking, I will work on the quality until it's ready for merging.

@Sarcasm
Copy link
Owner

Sarcasm commented Aug 11, 2015

In what way does CompilationDatabase::constructFileNames rely on -c? It simply takes the first compilation argument not prefixed by a dash and skips the output file if it is present. I'm aware this is probably not the perfect solution, and I will update the details when I have aquired data from different systems/projects as test cases.

I mean, looking up the command line is error prone while the file is already provided in JSON.

If the Clang binding to the compilation database loses information for us, maybe we will be better of using simply a JSON parser.

The files are linked to their compilation commands through clang_CompilationDatabase_getCompileCommands. I assume the database is a map, in which case this should be fine.

make sense

More importantly, what do you think of the general approch? I think obtaining the file names from the compile commands is the right way to go, even if it needs more work than this.

I don't think it's the right way. The right way is to read the JSON compilation database with a JSON parser I think. We will have to guess header files. I would prefer we don't guess what is provided in addition to that.

I'd rather have a proof of concept first, both for caching and guessing. When I see you agree on the direction my work is taking, I will work on the quality until it's ready for merging.
Hide all checks

I'm still not convinced. It will be easy for me to review caching. Guessing may be more subject to discussion. And both together will be a lot of discussion and more difficult to review.

@Hylen
Copy link
Contributor Author

Hylen commented Aug 11, 2015

I mean, looking up the command line is error prone while the file is already provided in JSON.

If the Clang binding to the compilation database loses information for us, maybe we will be better of using simply a JSON parser.

I agree. This was because you wanted to be restrictive about adding dependencies. Do you have a good candidate for the JSON parser? I gave RapidJSON a quick look just now.

I'm still not convinced. It will be easy for me to review caching. Guessing may be more subject to discussion. And both together will be a lot of discussion and more difficult to review.

Alright, argument accepted. I will implement the database caching before the guessing algorithm. I have a good idea of what you think now anyway.

@Sarcasm
Copy link
Owner

Sarcasm commented Aug 11, 2015

Do you have a good candidate for the JSON parser? I gave RapidJSON a quick look just now.

I have a list, RapidJSON is in it but doesn't fit the bill IMHO. It supports on Clang >= 3.4 while I have been careful to stay as backward compatible as possible. Also I would like to drop the file into irony-source code if possible (I usually don't do this but I prefer irony to be self-contained for now).

With these constraints in mind, you may want to take a look at https://github.com/miloyip/nativejson-benchmark#libraries

@Hylen
Copy link
Contributor Author

Hylen commented Aug 12, 2015

It supports on Clang >= 3.4 while I have been careful to stay as backward compatible as possible.

What do you mean? Since there will be no direct interaction between clang and the JSON parser it shouldn't matter which version we use. Especially if we drop it in the irony-mode source code. RapidJSON is apparently a header-only library, so it should be easy to drop it in too.

If you have a lot of requirements on the parser we use, it may actaullay be easier if you make the decision before I continue to implement the database.

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.

2 participants