Page MenuHomePhabricator

Added functions to retrieve information about whether a vardecl is local in libclang and its python bindings.
Needs ReviewPublic

Authored by RedX2501 on Jun 30 2015, 12:06 AM.

Details

Reviewers
akyrtzi
rsmith

Diff Detail

Event Timeline

RedX2501 updated this revision to Diff 28758.Jun 30 2015, 12:06 AM
RedX2501 retitled this revision from to Added functions to retrieve information about variable storage in libclang and its python bindings..
RedX2501 updated this object.
RedX2501 edited the test plan for this revision. (Show Details)
RedX2501 added a subscriber: Unknown Object (MLST).

The code owners file says you are the owner of all parts that do not belong to somebody else. So i hope it is ok if i attach you as reviewer to this.

rsmith edited reviewers, added: rsmith; removed: doug.gregor.Oct 8 2015, 3:32 PM
rsmith added inline comments.Oct 8 2015, 3:50 PM
include/clang-c/Index.h
3884–3890

It might be better to combine these into a single function (getLocalVarKind?) returning an enum { not local, non-static local, static local }.

RedX2501 added inline comments.Oct 11 2015, 8:54 PM
include/clang-c/Index.h
3884–3890

Combining these into one function with an enum is much more work when porting to python (which is also den in this patch). Therefore unless there are reasons beyond stylistic ones i'd prefer to keep them separate.

IMHO this also increases the friendliness as you can directly infer from the documentation at http://clang.llvm.org/doxygen/ what they should do as they just forward the calls..

rsmith edited edge metadata.

Argyrios, I'd appreciate your thoughts here.

tools/libclang/CIndex.cpp
6924–6948

I don't think the names of these are really specific enough for what they do. As members of VarDecl, they're good enough, but as operations on a general Cursor, it's less so. For instance, temporary objects in C++ might also have local storage or be static locals. Renaming isStaticLocal to isStaticLocalVar would help.

hasLocalStorage is not really a very user-friendly name for this functionality, even though it currently matches our C++ API (the C API has long-term stability guarantees whereas the C++ API does not, so we need to take more care when naming C API functions, even though matching the C++ API does generally make the C API more user-friendly). isStaticLocalVar versus isNonStaticLocalVar might be better if you don't want to go the enum route.

RedX2501 updated this revision to Diff 39163.EditedNov 3 2015, 9:01 PM
RedX2501 retitled this revision from Added functions to retrieve information about variable storage in libclang and its python bindings. to Added functions to retrieve information about whether a vardecl is local in libclang and its python bindings..
RedX2501 edited edge metadata.

No longer export two functions but only one.

On further thought I only need the information is the variable was declared local or not. I don't really need the information where the storage ultimately is.

Added python and libclang tests.

akyrtzi edited edge metadata.Dec 9 2015, 12:18 PM
akyrtzi added a subscriber: akyrtzi.

The patch does not apply cleanly, could you provide a more up-to-date one ?

RedX2501 updated this revision to Diff 52358.Apr 1 2016, 6:18 AM
RedX2501 edited edge metadata.

Fixed compiler warnings.

RedX2501 updated this revision to Diff 52367.Apr 1 2016, 6:57 AM

Fix compilation error introduced due to breaking a long string.
Fixed indentation.