[PATCH] D127284: [clang-repl] Support statements on global scope in incremental mode.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5555-5568
+  if (SS.isNotEmpty() && SS.getScopeRep()) {
+    NestedNameSpecifier *NNS = SS.getScopeRep();
+    if (NNS->getAsNamespace() || NNS->getAsNamespaceAlias()) {
+      TPA.Revert();
+      return false;
+    }
+    if (CtorII) {
----------------
I don't think any of this should be necessary. Per the function comment, it's a precondition of this function that we're looking at either `ClassName` or `ClassNameScope::ClassName` and we shouldn't be re-checking that.

I also think we should not be returning `true` here if we see `ClassName::ClassName` without looking at the rest of the tokens. That'll cause diagnostic quality regressions like the one you're seeing. We should continue to the rest of this function to see if we have `(Type` or `()` next.


================
Comment at: clang/lib/Parse/ParseTentative.cpp:61
+      if (getLangOpts().CPlusPlus) {
+        if (isConstructorDeclarator(/*Unqualified=*/false,
+                                    /*DeductionGuide=*/false,
----------------
v.g.vassilev wrote:
> aaron.ballman wrote:
> > Do you need special handling for:
> > ```
> > struct S {
> >   operator int();
> > };
> > 
> > S::operator int() { return 0; }
> > ```
> We seem to need that. Thanks. I've added a FIXME.
I think you should check `isCurrentClassName` before calling this, like the other callers of this function do. My understanding is that the intent is for `isConstructorDeclarator` to only check the (potential) function declarator that appears after the name, not to check the name itself.


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

https://reviews.llvm.org/D127284