sammccall added a comment.
Thanks a lot, great comments!
I haven't made changes yet (I will) but there's a few questions to work out
first...
================
Comment at: lib/Lex/Lexer.cpp:1931
+ StringRef PartialPath(AfterQuote, CurPtr - 1 - AfterQuote);
+ auto Slash = PartialPath.rfind('/');
+ StringRef Dir =
----------------
ilya-biryukov wrote:
> 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`.
Ah yuck, that's true (for some value of "valid"). Pre-C++11 it was explicitly
UB...
So if a codebase consistently uses backslashes, we're going to break that style
at least sometimes: we can't know whether to complete `#include <foo` as
`<foobar/` or `<foobar\`.
Any objection to just normalizing slashes for the whole include as part of
completion?
e.g. `#include <foo\ba` would complete as `#include <foo/bar/` and `#include
<foo/baz.h>`?
Anything else is going to be fiddly, probably only help a few users a little
bit, and leave edge cases.
================
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 <
----------------
ilya-biryukov wrote:
> 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?
Ah no, it doesn't, though I thought so at first.
First: the address of `C` is `CurPtr-1` (getAndAdvanceChar is in spirit `return
*CurPtr++`).
Second: the buffer is null-terminated one-past-the-end (`*BufferEnd == 0`) and
**also** has an embedded null at the completion point. So when completing at
EOF it looks like:
```
# i n c l u d e < a \0 \0
^C ^CurPtr
^BufferStart ^BufPtr^BufferEnd
```
and we hit the case below. We only hit this case if we never saw a completion
point.
================
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);
----------------
ilya-biryukov wrote:
> 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/^"
> ```
Don't editors that add quotes/parens typically avoid duplicating them when
they're typed/completed?
This isn't actually easy to do, because after completing `file.h` you want the
cursor after the closing quote, and after completing `dir/` you want the cursor
inside the closing quote. I don't see a way to do this with
CodeCompletionString other than generating the quote in the former case but not
the latter. (At least not one that doesn't break something else...)
================
Comment at: lib/Sema/SemaCodeComplete.cpp:8028
+ CodeCompleter->getCodeCompletionTUInfo());
+ Builder.AddTextChunk(InternedPrefix);
+ Builder.AddTypedTextChunk(InternedTyped);
----------------
ilya-biryukov wrote:
> 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>`.
I tried a few iterations here and this was the only one that didn't seem
totally odd. Basically it's about the fact that these are quoted strings.
Ergonomically, having the closing quote completed after a filename *feels*
right - you can hit enter afterwards and go to the next line.
If the closing quote is inserted, it has to be displayed (can't be avoided,
also would be confusing).
So if you make the completion range just the filename, you end up with a
completion list like:
```
#include <foo/ba^`
bar.h>
baz/
backwards.h>
```
vs the current
```
#include <foo/ba^`
<foo/bar.h>
<foo/baz/
<foo/backwards.h>
```
So I see your point here but I'm not sure the other behavior is actually better.
================
Comment at: lib/Sema/SemaCodeComplete.cpp:8057
+ if (!(Filename.endswith(".h") || Filename.endswith(".hh") ||
+ Filename.endswith(".H") || Filename.endswith(".hpp") ||
+ Filename.endswith(".inc")))
----------------
ilya-biryukov wrote:
> Maybe do case-insensitive matching?
> A corner case, but still...
This matches Driver's behavior (I'd reuse it but I don't think Sema can/should
depend on Driver). Clang doesn't think "HPP" is a valid c++ header filename.
I'll add a comment.
Repository:
rC Clang
https://reviews.llvm.org/D52076
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
