[PATCH] D52076: [CodeComplete] Add completions for filenames in #include directives.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Amazing, can't wait for it to land!



================
Comment at: lib/Lex/Lexer.cpp:1931
+          StringRef PartialPath(AfterQuote, CurPtr - 1 - AfterQuote);
+          auto Slash = PartialPath.rfind('/');
+          StringRef Dir =
----------------
It's also valid to use `\` for path separators on Windows, maybe also handle those?
Clang also supports this when running e.g. `clang-cl /c foo.cpp`.


================
Comment at: lib/Lex/Lexer.cpp:2065
+    if (C == '\n' || C == '\r' ||                // Newline.
+        (C == 0 && (CurPtr - 1 == BufferEnd))) { // End of file.
       // If the filename is unterminated, then it must just be a lone <
----------------
Does that mean we're losing completion at the end of file?
Not sure if it's a big deal. The only common pattern I can come up with is starting with an empty file and writing:
```
#include "^
``` 

Should we handle that? WDYT?


================
Comment at: lib/Lex/Lexer.cpp:2075
+        // Completion only applies to the filename, after the last slash.
+        StringRef PartialPath(AfterLessPos, CurPtr - 1 - AfterLessPos);
+        auto Slash = PartialPath.rfind('/');
----------------
This code looks exactly the same between two functions.
Maybe extract it into a helper function?


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8021
+    // Directory completion is up to the slash, e.g. <sys/
+    TypedChunk.push_back(IsDirectory ? '/' : Angled ? '>' : '"');
+    auto R = SeenResults.insert(TypedChunk);
----------------
Could we avoid adding `/`, `>` and `"` if it's currently missing?
Otherwise it'll be annoying for editors that auto-add closing quotes and completions when editing existing includes, i.e.
```
// It's cool if we complete bar.h" (with closing quote) here:
#include "foo/^
// It's annoying if we complete an extra closing quote here:
#include "foo/^"
```


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8028
+                                    CodeCompleter->getCodeCompletionTUInfo());
+      Builder.AddTextChunk(InternedPrefix);
+      Builder.AddTypedTextChunk(InternedTyped);
----------------
Maybe leave this out?
When completing `std::^` the completion is `vector`, not `std::vector`.
In the same spirit, for includes I would expect completions for `#include <foo/^>` to be `bar.h`, not `<foo/bar.h>`.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8046
+         !EC && It != vfs::directory_iterator(); It.increment(EC)) {
+      if (++Count == 1000) // If we happen to hit a huge directory,
+        break;             // bail out early so we're not too slow.
----------------
The size of `/usr/include` on my machine is ~830 files and dirs, which is a little close to a limit.
Not sure if the number of files grow with time or with a number of installed packages, but maybe we want a slightly higher limit to make sure it's won stop showing some results from this important include dir anytime soon?


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8057
+          if (!(Filename.endswith(".h") || Filename.endswith(".hh") ||
+                Filename.endswith(".H") || Filename.endswith(".hpp") ||
+                Filename.endswith(".inc")))
----------------
Maybe do case-insensitive matching?
A corner case, but still...


================
Comment at: lib/Sema/SemaCodeComplete.cpp:8076
+      // header maps are not (currently) enumerable.
+      break;
+    case DirectoryLookup::LT_NormalDir:
----------------
NIT: Maybe return from each branch and put llvm_unreachable at the end of the function?
To get an error in case the new enum value is added in addition to the compiler warning.

Feel free to ignore if you like the current version more, the compiler warning is not likely to go unnoticed anyway.


Repository:
  rC Clang

https://reviews.llvm.org/D52076