[PATCH] D58062: Support framework import/include auto-completion

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Code looks good apart from one case where we encounter Foo.framework/Subdir1/Subdir2.

Can you add a test to `clang/test/CodeCompletion`? `included-files.cpp` has the existing tests, not sure if you can add them there or it needs to be an obj-c file for framework includes.



================
Comment at: lib/Sema/SemaCodeComplete.cpp:8404
+        if (LookupType == DirectoryLookup::LT_Framework) {
+          if (!Filename.endswith(".framework")) {
+            break;
----------------
nit: no {} needed for these one-line `if`s


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8404
+        if (LookupType == DirectoryLookup::LT_Framework) {
+          if (!Filename.endswith(".framework")) {
+            break;
----------------
sammccall wrote:
> nit: no {} needed for these one-line `if`s
this check appears to be incorrect, it'll fire even if NativeRelDir is nonempty (i.e. a we're inside a framework already.)
I think this can be combined with the one below it, see next comment.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8407
+          }
+          if (NativeRelDir.empty()) {
+            Filename.consume_back(".framework");
----------------
`consume_back` checks for the suffix, just `if (NativeRelDir.empty() && !Filename.consume_back(".framework")) break;` is enough


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58062/new/

https://reviews.llvm.org/D58062