Page MenuHomePhabricator

libclang plugin patch
Needs ReviewPublic

Authored by realincubus on Oct 3 2014, 3:53 PM.

Details

Summary

The goal is to enable tools that use libclang to parse c/c++ code to display additional errors and warnings emitted by plugins.
Normally libclang does not load any plugins when its parsing code through clang_parseTranslationUnit.
This patch moves the plugin loading code from ExecuteCompilerInvocation to the class CompilerInstance and
adds the method calls to ASTUnit::Parse and ExecuteCompilerInvocation.
I additionally added the symbols that are needed for the plugin to link against libclang to tools/libclang/libclang.exports.
If one links the plugin against libclang it works from the vim plugin YouCompleteMe and clang itself.

Diff Detail

Event Timeline

realincubus updated this revision to Diff 14413.Oct 3 2014, 3:53 PM
realincubus retitled this revision from to libclang plugin patch.
realincubus updated this object.
realincubus edited the test plan for this revision. (Show Details)
realincubus updated this object.Oct 3 2014, 4:00 PM
realincubus added a subscriber: Unknown Object (MLST).
klimek edited edge metadata.Oct 6 2014, 12:57 AM

Thanks a lot for working on this!

  1. Some general high level feedback inline
  2. Can you please upload the next patch with full context (http://llvm.org/docs/Phabricator.html)

With svn, you do that by typing:
$ svn diff --diff-cmd=diff -x -U999999

examples/PrintFunctionNames/CMakeLists.txt
13

The other code around doesn't seem to have spaces in parenthesis. The general guideline in llvm is to be consistent with the code around you - that makes reviewing much more pleasurable ;)

examples/PrintFunctionNames/PrintFunctionNames.cpp
26–42

Can you clang-format over it.

31

I think having a field in the middle between methods leads to it getting lost. Also, does this have to be public?

There are basically 2 ways in llvm/clang to sort classes:
Either do it Java-style (private members first, then public methods), or the style I prefer that has all public methods up top, and then a private: section with private variables in the end. In each section, be consistent about how you group your methods and members; I personally like to have the methods first, then the fields, but generally, be consistent, and have some visible structure.

realincubus updated this revision to Diff 14442.Oct 6 2014, 3:18 AM
realincubus edited edge metadata.
realincubus updated this revision to Diff 14443.Oct 6 2014, 3:24 AM

last patch was against the wrong revision.

With the nits fixed, it generally looks good from my side. The question is who the right owner is to give the good to go. Looping in Richard to delegate to the right person.

examples/PrintFunctionNames/PrintFunctionNames.cpp
26

No need for the _. CI(CI) works fine (and does TheRightThing).

include/clang/Frontend/CompilerInstance.h
713

Please make comments start with a capital letter.
Also, new doxygen comments should start with \brief
/// \brief Loads plugins that were added by -load.

lib/Frontend/CompilerInstance.cpp
1626

This comment seems superfluous :)

lib/FrontendTool/ExecuteCompilerInvocation.cpp
182–183

As does this now.

realincubus updated this revision to Diff 14510.Oct 7 2014, 7:19 AM

Looping in people who can find the right approver for this.

chandlerc resigned from this revision.Mar 29 2015, 2:01 PM
chandlerc edited reviewers, added: doug.gregor; removed: chandlerc.

I'm definitely not the right reviewer for this. Richard or Doug make much more sense.

klimek added a comment.Jul 3 2015, 6:46 AM

What's the state of this?

klimek added a subscriber: klimek.Jul 7 2015, 2:16 AM

Sounds good!