Clang tidy utility to generate a fix hint for a subsequent expression to the existing one

[barcisz]

Summary

This is a utility function for adding a statement to be executed after a specified one using FixItHints. It is designed to provide good formatting and protection against lack of curly brackets (for example when your preceding statement is in a for loop). This is a prerequisite to D133956 - one of the new CUDA checks. It also relies on D133725 to be able to search for comments right after a certain token.

[barcisz]

Use the new Lexer option to include comments while finding next token

[barcisz]

rebase

[njames93]

Would I be correct in assuming you have uploaded the wrong diff here?

[barcisz]

In D133942#3793618, @njames93 wrote:

Would I be correct in assuming you have uploaded the wrong diff here?

Yes, sorry, got mixed up with D133956 (6 diffs ain't easy to shuffle :-/)

[barcisz]

Misplaced diff

[njames93]

There's some merit in this, like wrapping the previous statement in braces.. However, clang-tidy should not focus on the formatting aspect as we have clang-format built in to address formatting decisions, and any special formatting you do in here would likely be undone by the users configuration.
It would also be nice to add in some unittests to demonstrate that braces are currently inserted etc.

How does this handle pathological cases like the statement being the iteration-expression of a for loop, or a init-statement in an if/switch/range-for loop. The documentation looks like it tries to explain that, but it doesn't do a great job IMHO.

[barcisz]

In D133942#3797449, @njames93 wrote:

It would also be nice to add in some unittests to demonstrate that braces are currently inserted etc.

Right sorry, forgot to port unit tests, will do that swiftly

[barcisz]

In D133942#3797449, @njames93 wrote:

How does this handle pathological cases like the statement being the iteration-expression of a for loop, or a init-statement in an if/switch/range-for loop. The documentation looks like it tries to explain that, but it doesn't do a great job IMHO.

It doesn't really, it's mostly meant for cases where we know that the statement will be in the body and it's only added in the utils because it's needed for D133956, but I can just move it to inside the check if you believe that it won't be as useful for the general case. The reason I want to preserve the statement to which the comments relate to is because if I put the new statement/macro invocation right after the current statement then the formatted will move the comments to the new line with the new statement instead of keeping them in the same line as the current statement (and in case of macros I'm not sure if it will even move it to the next line at all)

[barcisz]

Tests for the utility

clang-tools-extra/clang-tidy/utils/FixItHintUtils.h

 
 /// \brief Creates fix to qualify ``VarDecl`` with the specified \c Qualifier.
 /// Requires that `Var` is isolated in written code like in `int foo = 42;`.
 Optional<FixItHint>
 addQualifierToVarDecl(const VarDecl &Var, const ASTContext &Context,
                       DeclSpec::TQ Qualifier,
                       QualifierTarget CT = QualifierTarget::Pointee,
                       QualifierPolicy CP = QualifierPolicy::Left);
+
+/// \brief Adds a statement to be executed right after this statement .
+/// Is designed for taking potential comments or statements in the same line
+/// into account. The statement should not be an expression that's part of
+/// another statement. The statement range should include the terminator
+/// (semicolon).
+llvm::SmallVector<FixItHint, 1>
+addSubsequentStatement(SourceRange stmtRangeWithTerminator,
+                       const Stmt &parentStmt, llvm::StringRef nextStmt,
+                       ASTContext &context);
+
 } // namespace fixit
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_FIXITHINTUTILS_H

clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp

if (ParenStrippedType->isArrayType()) {
 
     if (AT->isPointerType())
       return changePointer(Var, Qualifier, AT->getPointeeType().getTypePtr(),
                            QualTarget, QualPolicy, Context);
   }
 
   return None;
 }
+
+static std::string getIndent(SourceLocation sLoc, ASTContext& context) {
+  auto& SM = context.getSourceManager();
+
+  const auto sLocLineNo = getLineNumber(sLoc, SM);
+
+  auto indentation_template = tooling::fixit::internal::getText(
+      CharSourceRange::getCharRange(SourceRange(
+          SM.translateLineCol(SM.getFileID(sLoc), sLocLineNo, 1), sLoc)),
+      context);
+
+  std::string indentation;
+  indentation.reserve(indentation_template.size());
+  std::transform(
+      indentation_template.begin(),
+      indentation_template.end(),
+      std::back_inserter(indentation),
+      [](char c) { return isspace(c) ? c : ' '; });
+  return indentation;
+}
+
+static llvm::SmallVector<FixItHint, 1> addSubsequentStatement(
+    SourceRange stmtRangeWithTerminator,
+    const Stmt& parentStmt,
+    llvm::StringRef nextStmt,
+    ASTContext& context) {
+  auto& SM = context.getSourceManager();
+  auto langOpts = context.getLangOpts();
+
+  const auto stmtEndLineNo =
+      getLineNumber(stmtRangeWithTerminator.getEnd(), SM);
+
+  // Find the first token's data for which the next token is
+  // either a line apart or is not a comment
+  SourceLocation lastTokenEndLoc =
+      stmtRangeWithTerminator.getEnd().getLocWithOffset(1);
+  auto lastTokenLine = stmtEndLineNo;
+  bool insertNewLine = true;
+  while (true) {
+    llvm::Optional<Token> tokenOption = findNextTokenIncludingComments(
+        lastTokenEndLoc.getLocWithOffset(-1), SM, langOpts);
+    if (!tokenOption) {
+      return llvm::SmallVector<FixItHint, 1>();
+    }
+    if (tokenOption->is(tok::eof)) {
+      insertNewLine = false;
+      break;
+    }
+    const auto tokenBeginLineNo = getLineNumber(tokenOption->getLocation(), SM);
+
+    if (tokenOption->isNot(tok::comment)) {
+      insertNewLine = tokenBeginLineNo != stmtEndLineNo;
+      break;
+    }
+    if (tokenBeginLineNo > lastTokenLine) {
+      break;
+    }
+
+    lastTokenEndLoc = tokenOption->getEndLoc();
+    lastTokenLine = getLineNumber(tokenOption->getEndLoc(), SM);
+  }
+
+  bool isEnclosedWithBrackets =
+      parentStmt.getStmtClass() == Stmt::CompoundStmtClass;
+
+  // Generating the FixItHint
+  // There are 5 scenarios that we have to take into account:
+  // 1. The statement is enclosed in brackets but the next statement is
+  //    in the same line - insert the new statement right after the previous one
+  // 2. The statement is not enclosed in brackets and the next statement is
+  //    in the same line - same as 1. and enclose both statements in brackets
+  //    on the same line
+  // 3. The statement is enclosed in brackets and the next statement is
+  //    on subsequent lines - skip all the comments in this line
+  // 4. The statement is not enclosed in brackets but the next statement is on
+  //    subsequent lines - same as 3. and enclose the statements with
+  //    google-style multiline brackets (opening bracket right after the parent
+  //    statement and closing bracket on a new line after the new statement).
+  // 5. The statement is not enclosed in brackets but the next statement is on
+  //    subsequent lines and the main statement is before an else token - same
+  //    as 4. but the closing bracket is put on the same line as the else
+  //    statement
+  if (!insertNewLine) {
+    if (isEnclosedWithBrackets) {
+      // Case 1.
+      return llvm::SmallVector<FixItHint, 1>{FixItHint::CreateInsertion(
+          stmtRangeWithTerminator.getEnd().getLocWithOffset(1),
+          (llvm::Twine(" ") + nextStmt.str() + ";").str())};
+    } else {
+      // Case 2.
+      return llvm::SmallVector<FixItHint, 1>{
+          FixItHint::CreateInsertion(stmtRangeWithTerminator.getBegin(), "{"),
+          FixItHint::CreateInsertion(
+              stmtRangeWithTerminator.getEnd().getLocWithOffset(1),
+              (llvm::Twine(" ") + nextStmt.str() + ";}").str())};
+    }
+  } else {
+    if (isEnclosedWithBrackets) {
+      // Case 3.
+      return llvm::SmallVector<FixItHint, 1>{FixItHint::CreateInsertion(
+          lastTokenEndLoc,
+          (llvm::Twine("\n") +
+           getIndent(stmtRangeWithTerminator.getBegin(), context) +
+           nextStmt.str() + ";")
+              .str())};
+    } else {
+      const auto previousTokenEndLoc =
+          tidy::utils::lexer::getPreviousToken(
+              stmtRangeWithTerminator.getBegin(), SM, context.getLangOpts())
+              .getEndLoc();
+      auto nextStmtIndent =
+          getIndent(stmtRangeWithTerminator.getBegin(), context);
+
+      if (getLineNumber(previousTokenEndLoc, SM) ==
+          getLineNumber(stmtRangeWithTerminator.getBegin(), SM)) {
+        nextStmtIndent += "  ";
+      }
+      auto nextToken = Lexer::findNextToken(lastTokenEndLoc, SM, langOpts);
+      if (!nextToken || nextToken->getRawIdentifier() != "else") {
+        // Case 4.
+        return llvm::SmallVector<FixItHint, 1>{
+            FixItHint::CreateInsertion(previousTokenEndLoc, " {"),
+            FixItHint::CreateInsertion(
+                lastTokenEndLoc,
+                (llvm::Twine("\n") + nextStmtIndent + nextStmt.str() + ";\n" +
+                 getIndent(parentStmt.getBeginLoc(), context) + "}")
+                    .str())};
+      } else {
+        // Case 5.
+        return llvm::SmallVector<FixItHint, 1>{
+            FixItHint::CreateInsertion(previousTokenEndLoc, " {"),
+            FixItHint::CreateInsertion(
+                lastTokenEndLoc,
+                (llvm::Twine("\n") + nextStmtIndent + nextStmt.str() + ";")
+                    .str()),
+            FixItHint::CreateInsertion(nextToken->getLocation(), "} ")};
+      }
+    }
+  }
+}
+
 } // namespace fixit
 } // namespace utils
 } // namespace tidy
 } // namespace clang