Enable clang-tidy modernization checks (#933)

This follows up on #924 by enabling the modernization set of
`clang-tidy` checks. As in the original PR, each individual commit
largely mechanically fixes simple issues related to a single check. The
changes here are stylistic ones that ensure we are using modern C++
features appropriately in the backend's code, and aren't leaning on
legacy / C-style practices.

There are a couple of commits that are not directly mechanical changes:
-
ed23fdd
allows the script that drives `clang-tidy` to receive additional CLI
args; doing so enables the use of the auto-fixer.
-
b74fe6a
refactors some formatting code to use FMT rather than legacy printing
functions.
-
e13d666
corrects the formatting of code generated by the auto-fixer.

As ever, I'm open to bikeshedding the specific changes introduced here -
it's really easy to change whether a specific check is enabled or
disabled. For instance, I have left the [trailing return
type](https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-trailing-return-type.html)
check disabled as it's not the code style we're using.

.clang-tidy

@@ -1,6 +1,8 @@
 Checks:
   clang-analyzer-*
   -clang-analyzer-cplusplus.PlacementNew
+  modernize-*
+  -modernize-use-trailing-return-type
   performance-*
   -performance-no-int-to-ptr
 

lib/ast/AST.cpp

@@ -4,6 +4,8 @@
 #include "kllvm/binary/serializer.h"
 #include "kllvm/parser/KOREParser.h"
 
+#include <fmt/format.h>
+
 #include <algorithm>
 #include <cassert>
 #include <cctype>
@@ -407,16 +409,16 @@ sptr<KOREPattern> KORECompositePattern::expandAliases(KOREDefinition *def) {
 static int indent = 0;
 static bool atNewLine = true;
 
-#define INDENT_SIZE 2
-
 static void newline(std::ostream &out) {
   out << std::endl;
   atNewLine = true;
 }
 
 static void printIndent(std::ostream &out) {
+  constexpr auto indent_size = 2;
+
   if (atNewLine) {
-    for (int i = 0; i < INDENT_SIZE * indent; i++) {
+    for (int i = 0; i < indent_size * indent; i++) {
       out << ' ';
     }
     atNewLine = false;
@@ -645,11 +647,10 @@ static void color(
 
 #define RESET_COLOR "\x1b[0m"
 
-std::string enquote(std::string str) {
+std::string enquote(const std::string &str) {
   std::string result;
   result.push_back('"');
-  for (size_t i = 0; i < str.length(); ++i) {
-    char c = str[i];
+  for (char c : str) {
     switch (c) {
     case '\\': result.append("\\\\"); break;
     case '"': result.append("\\\""); break;
@@ -661,11 +662,7 @@ std::string enquote(std::string str) {
       if ((unsigned char)c >= 32 && (unsigned char)c < 127) {
         result.push_back(c);
       } else {
-        char buf[3];
-        buf[2] = 0;
-        snprintf(buf, 3, "%02x", (unsigned char)c);
-        result.append("\\x");
-        result.append(buf);
+        fmt::format_to(std::back_inserter(result), "\\x{:02x}", c);
       }
       break;
     }
@@ -703,7 +700,7 @@ void KORECompositePattern::prettyPrint(
     std::ostream &out, PrettyPrintData const &data) const {
   std::string name = getConstructor()->getName();
   if (name == "\\dv") {
-    KORECompositeSort *s = dynamic_cast<KORECompositeSort *>(
+    auto *s = dynamic_cast<KORECompositeSort *>(
         getConstructor()->getFormalArguments()[0].get());
     bool hasHook = data.hook.count(s->getName());
     auto str = dynamic_cast<KOREStringPattern *>(arguments[0].get());
@@ -913,7 +910,7 @@ KORECompositePattern::sortCollections(PrettyPrintData const &data) {
       std::ostringstream Out;
       item = item->sortCollections(data);
       item->prettyPrint(Out, newData);
-      printed.push_back({Out.str(), item});
+      printed.emplace_back(Out.str(), item);
     }
     indent = oldIndent;
     atNewLine = oldAtNewLine;
@@ -940,7 +937,7 @@ KORECompositePattern::sortCollections(PrettyPrintData const &data) {
   return result;
 }
 
-std::set<std::string> KOREPattern::gatherSingletonVars(void) {
+std::set<std::string> KOREPattern::gatherSingletonVars() {
   auto counts = gatherVarCounts();
   std::set<std::string> result;
   for (auto const &entry : counts) {
@@ -951,7 +948,7 @@ std::set<std::string> KOREPattern::gatherSingletonVars(void) {
   return result;
 }
 
-std::map<std::string, int> KORECompositePattern::gatherVarCounts(void) {
+std::map<std::string, int> KORECompositePattern::gatherVarCounts() {
   std::map<std::string, int> result;
   for (auto &arg : arguments) {
     auto childResult = arg->gatherVarCounts();
@@ -962,7 +959,7 @@ std::map<std::string, int> KORECompositePattern::gatherVarCounts(void) {
   return result;
 }
 
-sptr<KOREPattern> KORECompositePattern::dedupeDisjuncts(void) {
+sptr<KOREPattern> KORECompositePattern::dedupeDisjuncts() {
   if (constructor->getName() != "\\or") {
     return shared_from_this();
   }
@@ -1657,7 +1654,7 @@ KOREAliasDeclaration::getSubstitution(KORECompositePattern *subject) {
   int i = 0;
   KOREPattern::substitution result;
   for (auto &arg : boundVariables->getArguments()) {
-    KOREVariablePattern *var = dynamic_cast<KOREVariablePattern *>(arg.get());
+    auto *var = dynamic_cast<KOREVariablePattern *>(arg.get());
     if (!var) {
       abort();
     }
@@ -1719,15 +1716,12 @@ void KOREDefinition::insertReservedSymbols() {
 void KOREDefinition::preprocess() {
   insertReservedSymbols();
 
-  for (auto iter = axioms.begin(); iter != axioms.end(); ++iter) {
-    auto axiom = *iter;
+  for (auto axiom : axioms) {
     axiom->pattern = axiom->pattern->expandAliases(this);
   }
   auto symbols = std::map<std::string, std::vector<KORESymbol *>>{};
   unsigned nextOrdinal = 0;
-  for (auto iter = symbolDeclarations.begin(); iter != symbolDeclarations.end();
-       ++iter) {
-    auto decl = *iter;
+  for (const auto &decl : symbolDeclarations) {
     if (decl.second->getAttributes().count("freshGenerator")) {
       auto sort = decl.second->getSymbol()->getSort();
       if (sort->isConcrete()) {
@@ -1747,11 +1741,10 @@ void KOREDefinition::preprocess() {
       ++iter;
     }
   }
-  for (auto moditer = modules.begin(); moditer != modules.end(); ++moditer) {
-    auto &declarations = (*moditer)->getDeclarations();
-    for (auto iter = declarations.begin(); iter != declarations.end(); ++iter) {
-      KORESymbolDeclaration *decl
-          = dynamic_cast<KORESymbolDeclaration *>(iter->get());
+  for (auto &module : modules) {
+    auto &declarations = module->getDeclarations();
+    for (const auto &declaration : declarations) {
+      auto *decl = dynamic_cast<KORESymbolDeclaration *>(declaration.get());
       if (decl == nullptr) {
         continue;
       }
@@ -1761,10 +1754,8 @@ void KOREDefinition::preprocess() {
       }
     }
   }
-  for (auto iter = symbols.begin(); iter != symbols.end(); ++iter) {
-    auto entry = *iter;
-    for (auto iter = entry.second.begin(); iter != entry.second.end(); ++iter) {
-      KORESymbol *symbol = *iter;
+  for (const auto &entry : symbols) {
+    for (auto symbol : entry.second) {
       auto decl = symbolDeclarations.at(symbol->getName());
       symbol->instantiateSymbol(decl);
     }
@@ -1775,11 +1766,9 @@ void KOREDefinition::preprocess() {
   auto layouts = std::unordered_map<std::string, uint16_t>{};
   auto variables
       = std::unordered_map<std::string, std::pair<uint32_t, uint32_t>>{};
-  for (auto iter = symbols.begin(); iter != symbols.end(); ++iter) {
-    auto entry = *iter;
+  for (const auto &entry : symbols) {
     uint32_t firstTag = nextSymbol;
-    for (auto iter = entry.second.begin(); iter != entry.second.end(); ++iter) {
-      KORESymbol *symbol = *iter;
+    for (auto symbol : entry.second) {
       if (symbol->isConcrete()) {
         if (!instantiations.count(*symbol)) {
           instantiations.emplace(*symbol, nextSymbol++);
@@ -1800,11 +1789,9 @@ void KOREDefinition::preprocess() {
           entry.first, std::pair<uint32_t, uint32_t>{firstTag, lastTag});
     }
   }
-  for (auto iter = symbols.begin(); iter != symbols.end(); ++iter) {
-    auto entry = *iter;
+  for (const auto &entry : symbols) {
     auto range = variables.at(entry.first);
-    for (auto iter = entry.second.begin(); iter != entry.second.end(); ++iter) {
-      KORESymbol *symbol = *iter;
+    for (auto symbol : entry.second) {
       for (auto &sort : symbol->getArguments()) {
         if (sort->isConcrete()) {
           hookedSorts[dynamic_cast<KORECompositeSort *>(sort.get())
@@ -1896,19 +1883,16 @@ void KORECompositePattern::print(std::ostream &Out, unsigned indent) const {
 }
 
 static std::string escapeString(const std::string &str) {
-  std::string result;
+  auto result = std::string{};
+
   for (char c : str) {
     if (c == '"' || c == '\\' || !isprint(c)) {
-      result.push_back('\\');
-      result.push_back('x');
-      char code[3];
-      snprintf(code, 3, "%02x", (unsigned char)c);
-      result.push_back(code[0]);
-      result.push_back(code[1]);
+      fmt::format_to(std::back_inserter(result), "\\x{:02x}", c);
     } else {
       result.push_back(c);
     }
   }
+
   return result;
 }
 

lib/ast/CMakeLists.txt

@@ -6,7 +6,7 @@ add_library(AST
 )
 
 target_link_libraries(AST
-  PUBLIC Parser BinaryKore)
+  PUBLIC fmt::fmt-header-only Parser BinaryKore)
 
 install(
   TARGETS AST

lib/binary/serializer.cpp

@@ -7,7 +7,7 @@ namespace detail {
 
 bool is_big_endian() {
   uint32_t i = 1;
-  uint8_t *c = reinterpret_cast<uint8_t *>(&i);
+  auto *c = reinterpret_cast<uint8_t *>(&i);
   return *c == 0x00;
 }
 
@@ -32,7 +32,7 @@ serializer::serializer(flags f)
 
 std::string serializer::byte_string() const {
   auto *ptr = reinterpret_cast<unsigned char const *>(buffer_.data());
-  return std::string(ptr, ptr + buffer_.size());
+  return {ptr, ptr + buffer_.size()};
 }
 
 void serializer::reset() {

lib/codegen/CreateStaticTerm.cpp

@@ -38,7 +38,7 @@ llvm::Constant *CreateStaticTerm::notInjectionCase(
   constructor->print(koreString);
   llvm::Constant *Block
       = Module->getOrInsertGlobal(koreString.str().c_str(), BlockType);
-  llvm::GlobalVariable *globalVar = llvm::dyn_cast<llvm::GlobalVariable>(Block);
+  auto *globalVar = llvm::dyn_cast<llvm::GlobalVariable>(Block);
 
   if (!globalVar->hasInitializer()) {
     std::vector<llvm::Constant *> blockVals;
@@ -146,8 +146,7 @@ CreateStaticTerm::createToken(ValueType sort, std::string contents) {
     llvm::Constant *global = Module->getOrInsertGlobal(
         "int_" + contents, llvm::StructType::getTypeByName(
                                Module->getContext(), INT_WRAPPER_STRUCT));
-    llvm::GlobalVariable *globalVar
-        = llvm::dyn_cast<llvm::GlobalVariable>(global);
+    auto *globalVar = llvm::dyn_cast<llvm::GlobalVariable>(global);
     if (!globalVar->hasInitializer()) {
       mpz_t value;
       const char *dataStart
@@ -159,8 +158,7 @@ CreateStaticTerm::createToken(ValueType sort, std::string contents) {
           = llvm::ArrayType::get(llvm::Type::getInt64Ty(Ctx), size);
       llvm::Constant *limbs
           = Module->getOrInsertGlobal("int_" + contents + "_limbs", limbsType);
-      llvm::GlobalVariable *limbsVar
-          = llvm::dyn_cast<llvm::GlobalVariable>(limbs);
+      auto *limbsVar = llvm::dyn_cast<llvm::GlobalVariable>(limbs);
       std::vector<llvm::Constant *> allocdLimbs;
       for (size_t i = 0; i < size; i++) {
         allocdLimbs.push_back(llvm::ConstantInt::get(
@@ -202,8 +200,7 @@ CreateStaticTerm::createToken(ValueType sort, std::string contents) {
     llvm::Constant *global = Module->getOrInsertGlobal(
         "float_" + contents, llvm::StructType::getTypeByName(
                                  Module->getContext(), FLOAT_WRAPPER_STRUCT));
-    llvm::GlobalVariable *globalVar
-        = llvm::dyn_cast<llvm::GlobalVariable>(global);
+    auto *globalVar = llvm::dyn_cast<llvm::GlobalVariable>(global);
     if (!globalVar->hasInitializer()) {
       size_t prec, exp;
       const char last = contents.back();
@@ -243,8 +240,7 @@ CreateStaticTerm::createToken(ValueType sort, std::string contents) {
           = llvm::ArrayType::get(llvm::Type::getInt64Ty(Ctx), size);
       llvm::Constant *limbs = Module->getOrInsertGlobal(
           "float_" + contents + "_limbs", limbsType);
-      llvm::GlobalVariable *limbsVar
-          = llvm::dyn_cast<llvm::GlobalVariable>(limbs);
+      auto *limbsVar = llvm::dyn_cast<llvm::GlobalVariable>(limbs);
       std::vector<llvm::Constant *> allocdLimbs;
       for (size_t i = 0; i < size; i++) {
         allocdLimbs.push_back(llvm::ConstantInt::get(
@@ -316,8 +312,7 @@ CreateStaticTerm::createToken(ValueType sort, std::string contents) {
          llvm::ArrayType::get(llvm::Type::getInt8Ty(Ctx), contents.size())});
     llvm::Constant *global
         = Module->getOrInsertGlobal("token_" + escape(contents), StringType);
-    llvm::GlobalVariable *globalVar
-        = llvm::dyn_cast<llvm::GlobalVariable>(global);
+    auto *globalVar = llvm::dyn_cast<llvm::GlobalVariable>(global);
     if (!globalVar->hasInitializer()) {
       llvm::StructType *BlockHeaderType = llvm::StructType::getTypeByName(
           Module->getContext(), BLOCKHEADER_STRUCT);

lib/codegen/CreateTerm.cpp

@@ -136,7 +136,7 @@ void addKompiledDirSymbol(
     bool debug) {
   auto Str = llvm::ConstantDataArray::getString(Context, dir, true);
   auto global = mod->getOrInsertGlobal(KOMPILED_DIR, Str->getType());
-  llvm::GlobalVariable *globalVar = llvm::cast<llvm::GlobalVariable>(global);
+  auto *globalVar = llvm::cast<llvm::GlobalVariable>(global);
   if (!globalVar->hasInitializer()) {
     globalVar->setInitializer(Str);
   }
@@ -411,7 +411,7 @@ llvm::Value *CreateTerm::createHook(
     assert(pattern->getArguments().size() == 2);
     llvm::Value *firstArg = alloc_arg(pattern, 0, locationStack);
     llvm::Value *secondArg = alloc_arg(pattern, 1, locationStack);
-    llvm::ICmpInst *Eq = new llvm::ICmpInst(
+    auto *Eq = new llvm::ICmpInst(
         *CurrentBlock, llvm::CmpInst::ICMP_EQ, firstArg, secondArg,
         "hook_BOOL_eq");
     return Eq;
@@ -432,14 +432,14 @@ llvm::Value *CreateTerm::createHook(
     llvm::Value *falseArg = alloc_arg(pattern, 2, locationStack);
     if (trueArg->getType()->isPointerTy()
         && !falseArg->getType()->isPointerTy()) {
-      llvm::AllocaInst *AllocCollection
+      auto *AllocCollection
           = new llvm::AllocaInst(falseArg->getType(), 0, "", CurrentBlock);
       new llvm::StoreInst(falseArg, AllocCollection, CurrentBlock);
       falseArg = AllocCollection;
     } else if (
         !trueArg->getType()->isPointerTy()
         && falseArg->getType()->isPointerTy()) {
-      llvm::AllocaInst *AllocCollection
+      auto *AllocCollection
           = new llvm::AllocaInst(trueArg->getType(), 0, "", NewTrueBlock);
       new llvm::StoreInst(trueArg, AllocCollection, NewTrueBlock);
       trueArg = AllocCollection;
@@ -686,7 +686,7 @@ llvm::Value *CreateTerm::createFunctionCall(
     case SortCategory::List:
     case SortCategory::Set: {
       if (!arg->getType()->isPointerTy()) {
-        llvm::AllocaInst *AllocCollection
+        auto *AllocCollection
             = new llvm::AllocaInst(arg->getType(), 0, "", CurrentBlock);
         new llvm::StoreInst(arg, AllocCollection, CurrentBlock);
         args.push_back(AllocCollection);
@@ -719,8 +719,8 @@ llvm::Value *CreateTerm::createFunctionCall(
   default: sret = false; break;
   }
   llvm::Value *AllocSret;
-  for (int i = 0; i < args.size(); i++) {
-    llvm::Value *arg = args[i];
+  types.reserve(args.size());
+  for (auto arg : args) {
     types.push_back(arg->getType());
   }
   std::vector<llvm::Value *> realArgs = args;
@@ -996,8 +996,7 @@ bool makeFunction(
   std::vector<llvm::Type *> paramTypes;
   std::vector<std::string> paramNames;
   std::vector<llvm::Metadata *> debugArgs;
-  for (auto iter = vars.begin(); iter != vars.end(); ++iter) {
-    auto &entry = *iter;
+  for (auto &entry : vars) {
     auto sort
         = dynamic_cast<KORECompositeSort *>(entry.second->getSort().get());
     if (!sort) {
@@ -1115,8 +1114,7 @@ std::string makeApplyRuleFunction(
   std::vector<llvm::Type *> paramTypes;
   std::vector<std::string> paramNames;
   std::vector<llvm::Metadata *> debugArgs;
-  for (auto iter = vars.begin(); iter != vars.end(); ++iter) {
-    auto &entry = *iter;
+  for (auto &entry : vars) {
     auto sort
         = dynamic_cast<KORECompositeSort *>(entry.second->getSort().get());
     if (!sort) {