[PATCH] D21502: Fix heuristics skipping invalid ctor-initializers with C++11

[Richard Smith via cfe-commits]

rsmith added a comment.

Please produce patches with more lines of context in future; phabricator only lets us comment on lines that are included in the patch, and in this case some of the relevant parts of the function are not in the context. (The equivalent of diff -U1000 is a common approach for this.)

Taking a step back, I wonder whether we have the right strategy overall for code completion within in-class //mem-initializer-list//s. The code after the code completion token is quite plausibly not even brace-balanced, in a case where you're writing a new constructor. Consider this completion:

  struct Foo {
    Foo() : some_long_x(0), some_|
    int some_long_x, some_long_y;
  };

Here, we ought to be able to complete "some_long_y", but we need to recognize that the next token is not part of the function definition in order to do that (and then we need to not try to consume a function body once we're done with the initializer). And conversely when completing here:

  struct Foo {
    Foo() : some_long_x(0), some_| {}
    int some_long_x, some_long_y;
  };

... we need to recognize that we //do// have a function body so that we can parse the members to find out what names we should complete.

However, this patch is an incremental improvement over what we already have, so I'm happy to go in this direction for now.


================
Comment at: lib/Parse/ParseCXXInlineMethods.cpp:840
@@ +839,3 @@
+
+      if (Tok.isOneOf(tok::identifier, tok::kw_template)) {
+        Toks.push_back(Tok);
----------------
Can you delete the check for `kw_template` here? We handle the `template` keyword above, and any time we actually hit this case we would expect `template` to be followed by one of `::`, `(`, or `{`, which makes no sense.

================
Comment at: lib/Parse/ParseCXXInlineMethods.cpp:847-851
@@ -849,2 +846,7 @@
     } while (Tok.is(tok::coloncolon));
 
+    if (Tok.is(tok::code_completion)) {
+      Toks.push_back(Tok);
+      ConsumeCodeCompletionToken();
+    }
+
----------------
You should probably also handle the case of a comma immediately after the code completion token, for completions like this:

  struct A {
    A() : new_mem|, existing_member() {}
    int new_member, existing_member;
  };

... and likewise the case where the completion token is followed by an identifier, a `::`, or a `decltype`, for completions like this:

  struct A {
    A() : new_mem| existing_member() {}
    int new_member, existing_member;
  };

In all those cases, I think you can just `continue` to pick up the rest of the initializers after the code completion token. (And if you `continue` from here in those cases, I think you can remove the handling of the code_completion token up on line 835, since this codepath will do the right thing in all those cases.)

================
Comment at: lib/Parse/ParseCXXInlineMethods.cpp:894-897
@@ +893,6 @@
+
+      const Token &PreviousToken = Toks[Toks.size() - 2];
+      if (!MightBeTemplateArgument &&
+          !PreviousToken.isOneOf(tok::identifier, tok::greater,
+                                 tok::greatergreater)) {
+        // Most likely the start of a function body rather than the start of a
----------------
Please add a comment here indicating that if the previous token is not one of these kinds, we've encountered an error (because this //mem-initializer// is missing its initializer). This is not really obvious, and it's important because that's what makes it correct to use a heuristic to guess whether we've got a function body next.


https://reviews.llvm.org/D21502