This is an archive of the discontinued LLVM Phabricator instance.

[Frontend] Avoid running plugins during code completion parse
ClosedPublic

Authored by nik on Apr 25 2018, 3:21 AM.

Details

Summary

The parsing that is done for code completion is a special case that will
discard any generated diagnostics, so avoid running plugins for this
case in the first place to avoid performance penalties due to the
plugins.

A scenario for this is for example libclang with extra plugins like tidy.

Diff Detail

Repository
rL LLVM

Event Timeline

nik created this revision.Apr 25 2018, 3:21 AM
nik updated this revision to Diff 143894.Apr 25 2018, 3:32 AM

only clang-format fixes

Seems reasonable; can you add a test for this (maybe somewhere in clang/test/Frontend/plugin*)?

I know very little about how code completion works, but it's not immediately obvious to me that disabling plugin ast consumers when code completion is enabled is necessarily correct. In what kind of scenario would we both have a plugin loaded that wants to insert an ast consumer before/after the main one, and also be doing code completion?

nik added a comment.Apr 26 2018, 2:36 AM

Seems reasonable; can you add a test for this (maybe somewhere in clang/test/Frontend/plugin*)?

Done.

I know very little about how code completion works, but it's not immediately obvious to me that disabling plugin ast consumers when code completion is enabled is necessarily correct. In what kind of scenario would we both have a plugin loaded that wants to insert an ast consumer before/after the main one, and also be doing code completion?

For example if you use libclang with the clang tidy plugin. I've put this into the commit message.

nik updated this revision to Diff 144084.Apr 26 2018, 2:37 AM

Added a test and clarified scenario in commit message.

nik added inline comments.Apr 26 2018, 2:46 AM
test/Frontend/plugins.c
7 ↗(On Diff #144084)

Should this test rather go into test/Index because of the c-index-test?

8 ↗(On Diff #144084)

Note that I actually have problems with this REQUIRES line. I use -DCLANG_BUILD_EXAMPLES and -DDBUILD_SHARED_LIBS=ON and this test (and the one above too) is skipped/unsupported. What else do I need?

Note that If I remove this line, the test is run - apparently the requirements are fulfilled, but not properly detected. I guess this is set up issue on my local machine?

nik edited the summary of this revision. (Show Details)Apr 26 2018, 3:52 AM
thakis added inline comments.May 3 2018, 12:24 PM
test/Frontend/plugins.c
7 ↗(On Diff #144084)

Yes, I think so.

8 ↗(On Diff #144084)

Are you on Windows? plugins is set here: http://llvm-cs.pcc.me.uk/tools/clang/test/lit.cfg.py#73
enable_shared here: http://llvm-cs.pcc.me.uk/tools/clang/test/lit.site.cfg.py.in#25
from here: http://llvm-cs.pcc.me.uk/cmake/modules/AddLLVM.cmake#1193

So my guess is you need to set LLVM_ENABLE_PLUGINS (and that you are on Windows), but I haven't tried it.

nik marked an inline comment as done.May 7 2018, 12:09 AM
nik added inline comments.
test/Frontend/plugins.c
8 ↗(On Diff #144084)

No, I'm not on Windows. Adding some debug-prints to the python scripts it looks like the "examples" thingy is actually not fulfilled here. Anyway, that's something for another change (will upload a separate change for this)

nik updated this revision to Diff 145430.May 7 2018, 12:10 AM

Moved the test to "Index".

nik added a comment.May 7 2018, 11:33 PM

OK, the issue with the test dependencies I've had is resolved by https://reviews.llvm.org/D46514

thakis accepted this revision.May 14 2018, 12:51 PM
thakis added inline comments.
test/Index/complete-and-plugins.c
3 ↗(On Diff #145430)

nit: usually the RUN lines go above the C code, not below it.

This revision is now accepted and ready to land.May 14 2018, 12:51 PM
nik marked an inline comment as done.May 16 2018, 12:37 AM
nik updated this revision to Diff 146999.May 16 2018, 12:38 AM

Addressed inline nit.

nik added a comment.May 16 2018, 12:39 AM

If this is fine now, please submit as I don't have the permissions to do so.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.