[PATCH] warning for restrict-qualified pointer assignments with undefined behavior

[Richard Smith]

The restriction on assigning between `restrict`-qualified pointers at the same level seems like a bug in the C spec to me. It makes no sense to me that

  int *restrict p = /*...*/;
  {
    int *restrict q = p;
  }

should be fine, but removing the braces should make it UB.

================
Comment at: lib/Sema/SemaExpr.cpp:8624
@@ +8623,3 @@
+    void VisitDeclRefExpr(DeclRefExpr *E) {
+      if (E->getDecl()->hasExternalFormalLinkage())
+        Vars.push_back(std::make_pair(E, S.getTUScope()));
----------------
I don't think this is what you mean. Linkage is not relevant to the `restrict` rules as far as I can see; I think you're trying to check the storage class here. Maybe `VarDecl::hasGlobalStorage()` is what you want?

================
Comment at: lib/Sema/SemaExpr.cpp:8626-8629
@@ +8625,6 @@
+        Vars.push_back(std::make_pair(E, S.getTUScope()));
+      else
+        for (Scope *DS = S.getCurScope(); DS; DS = DS->getParent())
+          if (DS->isDeclScope(E->getDecl()))
+            Vars.push_back(std::make_pair(E, DS));
+    }
----------------
Needs more braces. =)

================
Comment at: lib/Sema/SemaExpr.cpp:8634-8638
@@ +8633,7 @@
+      Scope *CS = nullptr;
+      for (Scope *SS = S.getCurScope(); SS; SS = SS->getParent())
+        if (SS->isClassScope()) {
+          CS = SS;
+          break;
+        }
+
----------------
I would think we should walk up to the block of the member function for a C++ `this` expression. Calling a `restrict`-qualified member function on a non-`restrict` object should only apply the restriction for the scope of the function.

================
Comment at: lib/Sema/SemaExpr.cpp:8640
@@ +8639,3 @@
+
+      assert(CS && "No class scope with a 'this' expression?");
+      Vars.push_back(std::make_pair((DeclRefExpr *) nullptr, CS));
----------------
I'd expect this assert to fire when defining a member function out of line.

================
Comment at: lib/Sema/SemaExpr.cpp:8646-8647
@@ +8645,4 @@
+      // The condition expression cannot contain the designating identifier.
+      Visit(E->getLHS());
+      Visit(E->getRHS());
+    }
----------------
You should only look at the chosen subexpression. Choose is supposed to be entirely transparent once its subexpression is chosen.

================
Comment at: lib/Sema/SemaExpr.cpp:8651-8653
@@ +8650,5 @@
+    void VisitArraySubscriptExpr(ArraySubscriptExpr *E) {
+      // The array subscript expression cannot contain the designating
+      // identifier.
+      Visit(E->getLHS());
+    }
----------------
You can't assume the array operand is on the left-hand side. `getBase`?

================
Comment at: lib/Sema/SemaExpr.cpp:8656-8659
@@ +8655,6 @@
+
+    void VisitStmt(Stmt *S) {
+      for (Stmt::child_range C = S->children(); C; ++C)
+        if (*C)
+          Visit(*C);
+    }
----------------
I don't think this is right in a lot of cases. For instance, you walk from a `CallExpr` to its arguments. Since you're going to tell the user their code has undefined behavior, you should err towards false negatives.

================
Comment at: lib/Sema/SemaExpr.cpp:8682-8683
@@ +8681,4 @@
+      if (E->getType().isRestrictQualified()) {
+	// This is a restrict-qualified member; to understand the scope, we
+	// need to find the designating identifier.
+        FindRestrictScope RS(S);
----------------
Weird indentation (here and elsewhere). Tabs?

================
Comment at: lib/Sema/SemaExpr.cpp:8811-8812
@@ +8810,4 @@
+  if (CompoundType.isNull() && LHSType.isRestrictQualified()) {
+    FindRestrictScope LHSRS(*this);
+    FindRestrictBasedOnScope RHSRS(*this);
+
----------------
You're using scopes pretty heavily here, but this code can be reached from template instantiation, where there are no scopes (and because `restrict` is part of a type, it's possible for the restrictness to not be known until instantiation). Maybe the easiest thing to do for now is to just skip all of this if we're performing template instantiation.

This stuff also won't work as-written with dependent types and dependent expressions.

http://reviews.llvm.org/D5889