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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8375
+  auto AddFilesFromIncludeDir = [&](StringRef IncludeDir, bool IsSystem,
+                                    bool isFramework) {
+    bool stripFrameworkSuffix = false;
----------------
nit: `IsFramework`, `StripFrameworkSuffix` etc


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8375
+  auto AddFilesFromIncludeDir = [&](StringRef IncludeDir, bool IsSystem,
+                                    bool isFramework) {
+    bool stripFrameworkSuffix = false;
----------------
sammccall wrote:
> nit: `IsFramework`, `StripFrameworkSuffix` etc
probably clearer just to pass in the `DirectoryLookup::LookupType_t`, rather than another bool.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8378
     llvm::SmallString<128> Dir = IncludeDir;
-    if (!NativeRelDir.empty())
+    if (isFramework) {
+      if (NativeRelDir.empty()) {
----------------
I don't think keeping the `stripFrameworkSuffix` state is worth scattering the reader's attention here.

I'd suggest instead:

```
if (!NativeRelDir.empty()) {
  if (LookupType == LT_Framework) {
    // In a framework directory, Foo/Bar/ is actually Foo.framework/Headers/Bar/.
    ...
  } else {
    append(Dir, NativeRelDir);
  }
}
```

or even pass Dir, NativeRelDir, and LookupType to a helper function to sort it out.

Below, you can just check NativeRelDir.empty() && type == Framework again...

If you do choose to keep it, please give it a descriptive name rather than an imperative one, e.g. `ListingFrameworksDir`


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8378
     llvm::SmallString<128> Dir = IncludeDir;
-    if (!NativeRelDir.empty())
+    if (isFramework) {
+      if (NativeRelDir.empty()) {
----------------
sammccall wrote:
> I don't think keeping the `stripFrameworkSuffix` state is worth scattering the reader's attention here.
> 
> I'd suggest instead:
> 
> ```
> if (!NativeRelDir.empty()) {
>   if (LookupType == LT_Framework) {
>     // In a framework directory, Foo/Bar/ is actually Foo.framework/Headers/Bar/.
>     ...
>   } else {
>     append(Dir, NativeRelDir);
>   }
> }
> ```
> 
> or even pass Dir, NativeRelDir, and LookupType to a helper function to sort it out.
> 
> Below, you can just check NativeRelDir.empty() && type == Framework again...
> 
> If you do choose to keep it, please give it a descriptive name rather than an imperative one, e.g. `ListingFrameworksDir`
this needs some explanation (mostly the why, not the how)


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8388
+          Framework += ".framework";
+          llvm::sys::path::append(Dir, Framework, "Headers");
+          llvm::sys::path::append(Dir, ++Begin, End);
----------------
just `append(Dir, *Begin + ".framework", "Headers")` ?
append takes twines I think.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8390
+          llvm::sys::path::append(Dir, ++Begin, End);
+        } else {
+          SmallString<32> Framework(NativeRelDir);
----------------
why is the if/else needed? the code above should cover both cases, the last append is a no-op if ++begin == end.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8409
       case llvm::sys::fs::file_type::directory_file:
-        AddCompletion(Filename, /*IsDirectory=*/true);
+        if (stripFrameworkSuffix && Filename.endswith(".framework")) {
+          auto FrameworkName = Filename.substr(0, Filename.size() - 10);
----------------
if a framework directory contains a subdirectory that does not end in ".framework", is it actually legal to include from it? Maybe we should be omitting it.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8410
+        if (stripFrameworkSuffix && Filename.endswith(".framework")) {
+          auto FrameworkName = Filename.substr(0, Filename.size() - 10);
+          AddCompletion(FrameworkName, /*IsDirectory=*/true);
----------------
```
if (should strip suffix)
  Filename.consume_back(".framework")
```
(no need for else)


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8410
+        if (stripFrameworkSuffix && Filename.endswith(".framework")) {
+          auto FrameworkName = Filename.substr(0, Filename.size() - 10);
+          AddCompletion(FrameworkName, /*IsDirectory=*/true);
----------------
sammccall wrote:
> ```
> if (should strip suffix)
>   Filename.consume_back(".framework")
> ```
> (no need for else)
one-line comment should be enough to explain the intent here: `// Entries in a framework directory have a .framework suffix, this does not appear in the source code`


Repository:
  rC Clang

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

https://reviews.llvm.org/D58062