rewrite_to_chrome_style: Make is-const decisions more consistent

This fixes cases where a templated method accesses a member on its
class object, and cases where a function argument is templated and
used as the input to a const variable.

R=dcheng@chromium.org
BUG=677285

Review-Url: https://codereview.chromium.org/2622003002
Cr-Commit-Position: refs/heads/master@{#442936}

tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

@@ -431,15 +431,36 @@ bool CanBeEvaluatedAtCompileTime(const clang::Stmt* stmt,
       return false;
   }
 
-  // If the expression is a template input then its coming at compile time so
-  // we consider it const. And we can't check isEvaluatable() in this case as
-  // it will do bad things/crash.
-  if (expr->isInstantiationDependent())
-    return true;
+  // If the expression depends on template input, we can not call
+  // isEvaluatable() on it as it will do bad things/crash.
+  if (!expr->isInstantiationDependent()) {
+    // If the expression can be evaluated at compile time, then it should have a
+    // kFoo style name. Otherwise, not.
+    return expr->isEvaluatable(context);
+  }
 
-  // If the expression can be evaluated at compile time, then it should have a
-  // kFoo style name. Otherwise, not.
-  return expr->isEvaluatable(context);
+  // We do our best to figure out special cases as we come across them here, for
+  // template dependent situations. Some cases in code are only considered
+  // instantiation dependent for some template instantiations! Which is
+  // terrible! So most importantly we try to match isEvaluatable in those cases.
+  switch (expr->getStmtClass()) {
+    case clang::Stmt::CXXThisExprClass:
+      return false;
+    case clang::Stmt::DeclRefExprClass: {
+      auto* declref = clang::dyn_cast<clang::DeclRefExpr>(expr);
+      auto* decl = declref->getDecl();
+      if (clang::dyn_cast<clang::VarDecl>(decl))
+        return false;
+      break;
+    }
+
+    default:
+      break;
+  }
+
+  // Otherwise, we consider depending on template parameters to not interfere
+  // with being const.. with exceptions hopefully covered above.
+  return true;
 }
 
 bool IsProbablyConst(const clang::VarDecl& decl,

tools/clang/rewrite_to_chrome_style/tests/template-expected.cc

@@ -76,6 +76,40 @@ void F() {
   const int kIsAConstToo = number;
 }
 
+namespace test_member_in_template {
+
+template <typename T>
+class HasAMember {
+ public:
+  HasAMember() {}
+  HasAMember(const T&) {}
+
+  void UsesMember() { const int not_const = i_; }
+  void AlsoUsesMember();
+
+ private:
+  int i_;
+};
+
+template <typename T>
+void HasAMember<T>::AlsoUsesMember() {
+  const int not_const = i_;
+}
+
+template <typename T>
+static void BasedOnSubType(const HasAMember<T>& t) {
+  const HasAMember<T> problematic_not_const(t);
+}
+
+void Run() {
+  HasAMember<int>().UsesMember();
+
+  BasedOnSubType<int>(HasAMember<int>());
+  enum E { A };
+  BasedOnSubType<E>(HasAMember<E>());
+}
+}
+
 namespace test_template_arg_is_function {
 
 void F(int x) {}

tools/clang/rewrite_to_chrome_style/tests/template-original.cc

@@ -75,6 +75,40 @@ void F() {
   const int isAConstToo = number;
 }
 
+namespace test_member_in_template {
+
+template <typename T>
+class HasAMember {
+ public:
+  HasAMember() {}
+  HasAMember(const T&) {}
+
+  void usesMember() { const int notConst = m_i; }
+  void alsoUsesMember();
+
+ private:
+  int m_i;
+};
+
+template <typename T>
+void HasAMember<T>::alsoUsesMember() {
+  const int notConst = m_i;
+}
+
+template <typename T>
+static void basedOnSubType(const HasAMember<T>& t) {
+  const HasAMember<T> problematicNotConst(t);
+}
+
+void Run() {
+  HasAMember<int>().usesMember();
+
+  basedOnSubType<int>(HasAMember<int>());
+  enum E { A };
+  basedOnSubType<E>(HasAMember<E>());
+}
+}
+
 namespace test_template_arg_is_function {
 
 void f(int x) {}