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

[Eric Liu via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL284011: [change-namespace] don't miss comments in the 
beginning of a namespace block. (authored by ioeric).

Changed prior to commit:
  https://reviews.llvm.org/D25397?vs=74038=74368#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D25397

Files:
  clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
  clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp

Index: clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
+++ clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -552,6 +552,38 @@
   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
Index: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
===
--- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
+++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
@@ -82,33 +82,42 @@
   return CurrentNs;
 }
 
-// FIXME: get rid of this helper function if this is supported in clang-refactor
-// library.
-SourceLocation getStartOfNextLine(SourceLocation Loc, const SourceManager ,
-  const LangOptions ) {
+static std::unique_ptr
+getLexerStartingFromLoc(SourceLocation Loc, const SourceManager ,
+const LangOptions ) {
   if (Loc.isMacroID() &&
   !Lexer::isAtEndOfMacroExpansion(Loc, SM, LangOpts, ))
-return SourceLocation();
+return nullptr;
   // Break down the source location.
   std::pair LocInfo = SM.getDecomposedLoc(Loc);
   // Try to load the file buffer.
   bool InvalidTemp = false;
   llvm::StringRef File = SM.getBufferData(LocInfo.first, );
   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(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 ,
+ const LangOptions ) {
+  std::unique_ptr Lex = getLexerStartingFromLoc(Loc, SM, LangOpts);
+  if (!Lex.get())
+return SourceLocation();
   llvm::SmallVector Line;
   // FIXME: this is a bit hacky to get ReadToEndOfLine work.
-  Lex.setParsingPreprocessorDirective(true);
-  Lex.ReadToEndOfLine();
+  Lex->setParsingPreprocessorDirective(true);
+  Lex->ReadToEndOfLine();
   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
+ : End.getLocWithOffset(1);
 }
 
 // Returns `R` with new range that refers to code after `Replaces` being
@@ -365,6 +374,23 @@
   }
 }
 
+static SourceLocation getLocAfterNamespaceLBrace(const NamespaceDecl *NsDecl,
+ const SourceManager ,
+ const LangOptions ) {
+  std::unique_ptr Lex =
+ 

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

[Haojian Wu via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: change-namespace/ChangeNamespace.cpp:387
+  Token Tok;
+  while (!Lex->LexFromRawLexer(Tok) && Tok.isNot(tok::TokenKind::l_brace)) {
+  }

ioeric wrote:
> hokein wrote:
> > Maybe we can use `findLocationAfterToken` here? 
> 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(...);
> 
I see. Thanks for the clarification.


https://reviews.llvm.org/D25397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Liu via cfe-commits]

ioeric added inline comments.



Comment at: change-namespace/ChangeNamespace.cpp:387
+  Token Tok;
+  while (!Lex->LexFromRawLexer(Tok) && Tok.isNot(tok::TokenKind::l_brace)) {
+  }

hokein wrote:
> Maybe we can use `findLocationAfterToken` here? 
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(...);



https://reviews.llvm.org/D25397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via cfe-commits]

hokein added inline comments.



Comment at: change-namespace/ChangeNamespace.cpp:387
+  Token Tok;
+  while (!Lex->LexFromRawLexer(Tok) && Tok.isNot(tok::TokenKind::l_brace)) {
+  }

Maybe we can use `findLocationAfterToken` here? 


https://reviews.llvm.org/D25397



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eric Liu via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: hokein.
ioeric added a subscriber: cfe-commits.

https://reviews.llvm.org/D25397

Files:
  change-namespace/ChangeNamespace.cpp
  unittests/change-namespace/ChangeNamespaceTests.cpp

Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- unittests/change-namespace/ChangeNamespaceTests.cpp
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -552,6 +552,38 @@
   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
Index: change-namespace/ChangeNamespace.cpp
===
--- change-namespace/ChangeNamespace.cpp
+++ change-namespace/ChangeNamespace.cpp
@@ -82,33 +82,42 @@
   return CurrentNs;
 }
 
-// FIXME: get rid of this helper function if this is supported in clang-refactor
-// library.
-SourceLocation getStartOfNextLine(SourceLocation Loc, const SourceManager ,
-  const LangOptions ) {
+static std::unique_ptr
+getLexerStartingFromLoc(SourceLocation Loc, const SourceManager ,
+const LangOptions ) {
   if (Loc.isMacroID() &&
   !Lexer::isAtEndOfMacroExpansion(Loc, SM, LangOpts, ))
-return SourceLocation();
+return nullptr;
   // Break down the source location.
   std::pair LocInfo = SM.getDecomposedLoc(Loc);
   // Try to load the file buffer.
   bool InvalidTemp = false;
   llvm::StringRef File = SM.getBufferData(LocInfo.first, );
   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(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 ,
+ const LangOptions ) {
+  std::unique_ptr Lex = getLexerStartingFromLoc(Loc, SM, LangOpts);
+  if (!Lex.get())
+return SourceLocation();
   llvm::SmallVector Line;
   // FIXME: this is a bit hacky to get ReadToEndOfLine work.
-  Lex.setParsingPreprocessorDirective(true);
-  Lex.ReadToEndOfLine();
+  Lex->setParsingPreprocessorDirective(true);
+  Lex->ReadToEndOfLine();
   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
+ : End.getLocWithOffset(1);
 }
 
 // Returns `R` with new range that refers to code after `Replaces` being
@@ -365,6 +374,23 @@
   }
 }
 
+static SourceLocation getLocAfterNamespaceLBrace(const NamespaceDecl *NsDecl,
+ const SourceManager ,
+ const LangOptions ) {
+  std::unique_ptr 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()
+