Page MenuHomePhabricator

[clangd] Support pseudo-obj expr, opaque values, and property references in findExplicitReferences()
ClosedPublic

Authored by sammccall on Jan 10 2020, 7:27 AM.

Diff Detail

Event Timeline

sammccall created this revision.Jan 10 2020, 7:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2020, 7:27 AM
sammccall marked an inline comment as done.Jan 10 2020, 7:31 AM
sammccall added inline comments.
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
596

For consistency with the targetDecl() tests in this file, and because I had an error in the testcase that threw me for a loop. I'll try to find a better solution project-wide though.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

dgoldman requested changes to this revision.Jan 10 2020, 8:34 AM
dgoldman added inline comments.
clang-tools-extra/clangd/FindTarget.cpp
668

Worth mentioning this is handled in add()'s Visitor's VisitObjCPropertyRefExpr?

791

Worth noting that these two functions are used currently for ObjC?

clang-tools-extra/clangd/unittests/FindTargetTests.cpp
1077

Would also be good to have a test for implicit property refs (e.g - (int)z; obj.z;) Or maybe this should go in the other diff?

This revision now requires changes to proceed.Jan 10 2020, 8:34 AM
sammccall updated this revision to Diff 240378.Sat, Jan 25, 7:03 AM
sammccall marked 5 inline comments as done.

more tests

clang-tools-extra/clangd/FindTarget.cpp
668

I don't think we should echo the code structure here, it could change.

791

These functions are used for ObjC because these node types are used for objC - documenting that belongs on the AST class.
(I guess it's not currently done because someone currently/previously had an ambition to use these for other things, thus the generic names)

Unit tests: fail. 62194 tests passed, 1 failed and 815 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_recursive/try_lock.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

dgoldman accepted this revision.Sat, Jan 25, 9:01 AM
This revision is now accepted and ready to land.Sat, Jan 25, 9:01 AM
ilya-biryukov added inline comments.Sun, Jan 26, 10:07 PM
clang-tools-extra/clangd/FindTarget.cpp
794

Should this be done by RecursiveASTVisitor instead (when shouldVisitImplicitCode() is false)?

If yes, maybe add a FIXME to this patch and try fixing in RAV in a follow-up?

clang-tools-extra/clangd/unittests/FindTargetTests.cpp
701

Why is this necessary? Making tests more complicated to avoid warnings does not look right.

712

See the comment about making tests more complicated to avoid warnings. Same question here and in other instances.

sammccall updated this revision to Diff 240809.Tue, Jan 28, 2:12 AM
sammccall marked 4 inline comments as done.

Address comments

This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 62195 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.