[change-namespace] don't miss comments in the beginning of a namespace block.

[hokein]

change-namespace/ChangeNamespace.cpp

387

Maybe we can use findLocationAfterToken here?

[ioeric]

change-namespace/ChangeNamespace.cpp

387

This doesn't seem to work in this case. Note that we are lexing from the beginning of NsDecl, which is namespace or inline, and the next token is not necessarily {.

/// \brief Checks that the given token is the first token that occurs after the
/// given location (this excludes comments and whitespace). Returns the location
/// immediately after the specified token. If the token is not found or the
/// location is inside a macro, the returned source location will be invalid.
SourceLocation Lexer::findLocationAfterToken(...);

[hokein]

LGTM.

change-namespace/ChangeNamespace.cpp

387

I see. Thanks for the clarification.

change-namespace/ChangeNamespace.cpp

while (!PartialNsNameSplitted.empty()) {
     if (PartialNsNameSplitted.back() != CurrentNs->getNameAsString())
       return nullptr;
     PartialNsNameSplitted.pop_back();
     CurrentContext = CurrentContext->getParent();
   }
   return CurrentNs;
 }
 
-// FIXME: get rid of this helper function if this is supported in clang-refactor
-// library.
-SourceLocation getStartOfNextLine(SourceLocation Loc, const SourceManager &SM,
-                                  const LangOptions &LangOpts) {
+static std::unique_ptr<Lexer>
+getLexerStartingFromLoc(SourceLocation Loc, const SourceManager &SM,
   if (Loc.isMacroID() &&
       !Lexer::isAtEndOfMacroExpansion(Loc, SM, LangOpts, &Loc))
-    return SourceLocation();
+    return nullptr;
   // Break down the source location.
   std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(Loc);
   // Try to load the file buffer.
   bool InvalidTemp = false;
   llvm::StringRef File = SM.getBufferData(LocInfo.first, &InvalidTemp);
   if (InvalidTemp)
-    return SourceLocation();
+    return nullptr;
 
   const char *TokBegin = File.data() + LocInfo.second;
   // Lex from the start of the given location.
-  Lexer Lex(SM.getLocForStartOfFile(LocInfo.first), LangOpts, File.begin(),
-            TokBegin, File.end());
+  return llvm::make_unique<Lexer>(SM.getLocForStartOfFile(LocInfo.first),
+                                  LangOpts, File.begin(), TokBegin, File.end());
+}
 
+// FIXME: get rid of this helper function if this is supported in clang-refactor
+// library.
+static SourceLocation getStartOfNextLine(SourceLocation Loc,
+                                         const SourceManager &SM,
+                                         const LangOptions &LangOpts) {
+  std::unique_ptr<Lexer> Lex = getLexerStartingFromLoc(Loc, SM, LangOpts);
+  if (!Lex.get())
+    return SourceLocation();
   llvm::SmallVector<char, 16> Line;
   // FIXME: this is a bit hacky to get ReadToEndOfLine work.
-  Lex.setParsingPreprocessorDirective(true);
-  Lex.ReadToEndOfLine(&Line);
+  Lex->setParsingPreprocessorDirective(true);
+  Lex->ReadToEndOfLine(&Line);
   auto End = Loc.getLocWithOffset(Line.size());
-  return SM.getLocForEndOfFile(LocInfo.first) == End ? End
-                                                     : End.getLocWithOffset(1);
+  return SM.getLocForEndOfFile(SM.getDecomposedLoc(Loc).first) == End
+             ? End
 }
 
 // Returns `R` with new range that refers to code after `Replaces` being
 // applied.
 tooling::Replacement
 getReplacementInChangedCode(const tooling::Replacements &Replaces,
                             const tooling::Replacement &R) {
   unsigned NewStart = Replaces.getShiftedCodePosition(R.getOffset());
if (const auto *NsDecl = Result.Nodes.getNodeAs<NamespaceDecl>("old_ns")) {
     const clang::Decl *Context = Result.Nodes.getNodeAs<clang::Decl>("dc");
     assert(Context && "Empty decl context.");
     clang::SourceRange CalleeRange = Call->getCallee()->getSourceRange();
     replaceQualifiedSymbolInDeclContext(Result, Context, CalleeRange.getBegin(),
                                         CalleeRange.getEnd(), Name);
   }
 }
 
+static SourceLocation getLocAfterNamespaceLBrace(const NamespaceDecl *NsDecl,
+                                                 const SourceManager &SM,
+                                                 const LangOptions &LangOpts) {
+  std::unique_ptr<Lexer> Lex =
+      getLexerStartingFromLoc(NsDecl->getLocStart(), SM, LangOpts);
+  assert(Lex.get() &&
+         "Failed to create lexer from the beginning of namespace.");
+  if (!Lex.get())
+    return SourceLocation();
+  Token Tok;
+  while (!Lex->LexFromRawLexer(Tok) && Tok.isNot(tok::TokenKind::l_brace)) {
+  }
+  return Tok.isNot(tok::TokenKind::l_brace)
+             ? SourceLocation()
+             : Tok.getEndLoc().getLocWithOffset(1);
+}
+
 // Stores information about a moved namespace in `MoveNamespaces` and leaves
 // the actual movement to `onEndOfTranslationUnit()`.
 void ChangeNamespaceTool::moveOldNamespace(
     const ast_matchers::MatchFinder::MatchResult &Result,
     const NamespaceDecl *NsDecl) {
   // If the namespace is empty, do nothing.
   if (Decl::castToDeclContext(NsDecl)->decls_empty())
     return;
 
   // Get the range of the code in the old namespace.
-  SourceLocation Start = NsDecl->decls_begin()->getLocStart();
+  SourceLocation Start = getLocAfterNamespaceLBrace(
+      NsDecl, *Result.SourceManager, Result.Context->getLangOpts());
+  assert(Start.isValid() && "Can't find l_brace for namespace.");
   SourceLocation End = NsDecl->getRBraceLoc().getLocWithOffset(-1);
   // Create a replacement that deletes the code in the old namespace merely for
   // retrieving offset and length from it.
   const auto R = createReplacement(Start, End, "", *Result.SourceManager);
   MoveNamespace MoveNs;
   MoveNs.Offset = R.getOffset();
   MoveNs.Length = R.getLength();
 

unittests/change-namespace/ChangeNamespaceTests.cpp

std::string Expected = "namespace na {\n"
                          "namespace y {\n"
                          "\n"
                          "class B {};\n"
                          "} // namespace y\n"
                          "} // namespace x\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, CommentsBeforeMovedClass) {
+  std::string Code = "namespace na {\n"
+                     "namespace nb {\n"
+                     "\n\n"
+                     "// Wild comments.\n"
+                     "\n"
+                     "// Comments.\n"
+                     "// More comments.\n"
+                     "class B {\n"
+                     "  // Private comments.\n"
+                     "  int a;\n"
+                     "};\n"
+                     "}\n"
+                     "}";
+  std::string Expected = "\n"
+                         "\n"
+                         "namespace x {\n"
+                         "namespace y {\n"
+                         "\n\n"
+                         "// Wild comments.\n"
+                         "\n"
+                         "// Comments.\n"
+                         "// More comments.\n"
+                         "class B {\n"
+                         "  // Private comments.\n"
+                         "  int a;\n"
+                         "};\n"
+                         "} // namespace y\n"
+                         "} // namespace x\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
 } // anonymous namespace
 } // namespace change_namespace
 } // namespace clang