Get more consistent decisions if a statement is constant in templates

tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // Changes Blink-style names to Chrome-style names. Currently transforms:
 //   fields:
 //     int m_operationCount => int operation_count_
 //   variables (including parameters):
 //     int mySuperVariable => int my_super_variable
 //   constants:
@@ skipping 349 matching lines @@
     if (!first_char && was_lowercase && is_uppercase)
       needs_underscore = true;
     was_lowercase = is_lowercase;
     was_uppercase = is_uppercase;
     first_char = false;
   }
   std::reverse(output.begin(), output.end());
   return output;
 }
 
+bool CanBeEvaluatedAtCompileTime(const clang::Stmt* stmt,
+                                 const clang::ASTContext& context) {
+  auto* expr = clang::dyn_cast<clang::Expr>(stmt);
+  if (!expr) {
+    // If the statement is not an expression then it's a constant.

[dcheng, 2017/01/07 00:45:49]

What would it be that makes it a constant?

[danakj, 2017/01/07 00:48:00]

switch, if, return, language stuff. Not things that change at runtime.

[dcheng, 2017/01/07 00:50:18]

I think we always pass in an Expr though, no? So this should just take an Expr,
and we can remove this if (it doesn't make sense for any of those to be
initializers, AFAIK)

[danakj, 2017/01/09 16:01:33]

Children of the expr are stmts though.

+    return true;
+  }
+
+  // Function calls create non-consistent behaviour. For some template
+  // instantiations they can be constexpr while for others they are not, which
+  // changes the output of isEvaluatable().
+  if (expr->hasNonTrivialCall(context))
+    return false;
+
+  // Recurse on children. If they are all const (or are uses of template
+  // input) then the statement can be considered const. For whatever reason the
+  // below checks can give different-and-less-consistent responses if we call
+  // them on a complex expression than if we call them on the most primitive
+  // pieces (some pieces would say false but the whole thing says true).
+  for (auto* child : expr->children()) {
+    if (!CanBeEvaluatedAtCompileTime(child, context))

[danakj, 2017/01/09 16:01:51]

IE here child is not an expr always.

+      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 can be evaluated at compile time, then it should have a
+  // kFoo style name. Otherwise, not.
+  return expr->isEvaluatable(context);
+}
+
 bool IsProbablyConst(const clang::VarDecl& decl,
                      const clang::ASTContext& context) {
   clang::QualType type = decl.getType();
   if (!type.isConstQualified())
     return false;
 
   if (type.isVolatileQualified())
     return false;
 
   // Parameters should not be renamed to |kFooBar| style (even if they are
   // const and have an initializer (aka default value)).
   if (clang::isa<clang::ParmVarDecl>(&decl))
     return false;
 
   // http://google.github.io/styleguide/cppguide.html#Constant_Names
   // Static variables that are const-qualified should use kConstantStyle naming.
   if (decl.getStorageDuration() == clang::SD_Static)
     return true;
 
   const clang::Expr* initializer = decl.getInit();
   if (!initializer)
     return false;
 
-  // If the expression is dependent on a template input, then we are not
-  // sure if it can be compile-time generated as calling isEvaluatable() is
-  // not valid on |initializer|.
-  // TODO(crbug.com/581218): We could probably look at each compiled
-  // instantiation of the template and see if they are all compile-time
-  // isEvaluable().
-  if (initializer->isInstantiationDependent())
-    return false;
-
-  // If the expression can be evaluated at compile time, then it should have a
-  // kFoo style name. Otherwise, not.
-  return initializer->isEvaluatable(context);
+  return CanBeEvaluatedAtCompileTime(initializer, context);
 }
 
 AST_MATCHER_P(clang::QualType, hasString, std::string, ExpectedString) {
   return ExpectedString == Node.getAsString();
 }
 
 bool ShouldPrefixFunctionName(const std::string& old_method_name) {
   // Functions that are named similarily to a type - they should be prefixed
   // with a "Get" prefix.
   static const char* kConflictingMethods[] = {
@@ skipping 205 matching lines @@
     original_name = original_name.substr(strlen(kBlinkStaticMemberPrefix));
   else if (original_name.startswith(kBlinkFieldPrefix))
     original_name = original_name.substr(strlen(kBlinkFieldPrefix));
 
   bool is_const = IsProbablyConst(decl, context);
   if (is_const) {
     // Don't try to rename constants that already conform to Chrome style.
     if (original_name.size() >= 2 && original_name[0] == 'k' &&
         clang::isUppercase(original_name[1]))
       return false;
+    // Or names are spelt with underscore casing. While they are actually
+    // compile consts, the author wrote it explicitly as a variable not as
+    // a constant (they would have used kFormat otherwise here), so preserve
+    // it rather than try to mangle a kFormat out of it.
+    if (original_name.find('_') != StringRef::npos)
+      return false;
 
     name = 'k';
     name.append(original_name.data(), original_name.size());
     name[1] = clang::toUppercase(name[1]);
   } else {
     name = CamelCaseToUnderscoreCase(original_name);
 
     // Non-const variables with static storage duration at namespace scope are
     // prefixed with `g_' to reduce the likelihood of a naming collision.
     const clang::DeclContext* decl_context = decl.getDeclContext();
@@ skipping 817 matching lines @@
   for (const auto& r : replacements) {
     std::string replacement_text = r.getReplacementText().str();
     std::replace(replacement_text.begin(), replacement_text.end(), '\n', '\0');
     llvm::outs() << "r:::" << r.getFilePath() << ":::" << r.getOffset()
                  << ":::" << r.getLength() << ":::" << replacement_text << "\n";
   }
   llvm::outs() << "==== END EDITS ====\n";
 
   return 0;
 }