[clang-tools-extra] r296110 - [change-namepsace] make it possible to whitelist symbols so they don't get updated.

Eric Liu via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 28 02:14:03 PST 2017


I suspect it was caused by the new line splitting ("\r\n" vs "\n"?) on
Windows. In the test, I write some strings (expected to be new line
delimited) to a temp file (
https://github.com/llvm-mirror/clang-tools-extra/blob/master/test/change-namespace/white-list.cpp#L1),
read the content, and then split with "\n" (
https://github.com/llvm-mirror/clang-tools-extra/blob/master/change-namespace/tool/ClangChangeNamespace.cpp#L92).
I am trying trimming the lines to see if this fix the issue (r296458).

Unfortunately, I don't have a Windows machine available for testing so I
couldn't verify this locally.

- Eric

On Tue, Feb 28, 2017 at 10:28 AM Eric Liu <ioeric at google.com> wrote:

> Sorry for losing track of this.
>
> I thought I fixed it with r296113. Not sure why windows builds are still
> failing. I'll look into this.
>
> Thanks,
> Eric
>
> On Tue, Feb 28, 2017 at 2:19 AM Galina Kistanova <gkistanova at gmail.com>
> wrote:
>
> Hello Eric,
>
> This commit broke test on one of our builders:
>
>     Clang Tools :: change-namespace/white-list.cpp
>
>
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/6153/steps/test/logs/stdio
>
> Please have a look.
>
> Thanks
>
>
> Galina
>
>
> On Fri, Feb 24, 2017 at 3:54 AM, Eric Liu via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
> Author: ioeric
> Date: Fri Feb 24 05:54:45 2017
> New Revision: 296110
>
> URL: http://llvm.org/viewvc/llvm-project?rev=296110&view=rev
> Log:
> [change-namepsace] make it possible to whitelist symbols so they don't get
> updated.
>
> Reviewers: hokein
>
> Reviewed By: hokein
>
> Subscribers: cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D30328
>
> Added:
>     clang-tools-extra/trunk/test/change-namespace/Inputs/
>     clang-tools-extra/trunk/test/change-namespace/Inputs/fake-std.h
>     clang-tools-extra/trunk/test/change-namespace/white-list.cpp
> Modified:
>     clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
>     clang-tools-extra/trunk/change-namespace/ChangeNamespace.h
>     clang-tools-extra/trunk/change-namespace/tool/ClangChangeNamespace.cpp
>
> clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
>
> Modified: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp?rev=296110&r1=296109&r2=296110&view=diff
>
> ==============================================================================
> --- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp (original)
> +++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp Fri Feb
> 24 05:54:45 2017
> @@ -290,6 +290,7 @@ AST_MATCHER(EnumDecl, isScoped) {
>
>  ChangeNamespaceTool::ChangeNamespaceTool(
>      llvm::StringRef OldNs, llvm::StringRef NewNs, llvm::StringRef
> FilePattern,
> +    llvm::ArrayRef<std::string> WhiteListedSymbolPatterns,
>      std::map<std::string, tooling::Replacements> *FileToReplacements,
>      llvm::StringRef FallbackStyle)
>      : FallbackStyle(FallbackStyle),
> FileToReplacements(*FileToReplacements),
> @@ -308,6 +309,9 @@ ChangeNamespaceTool::ChangeNamespaceTool
>    }
>    DiffOldNamespace = joinNamespaces(OldNsSplitted);
>    DiffNewNamespace = joinNamespaces(NewNsSplitted);
> +
> +  for (const auto &Pattern : WhiteListedSymbolPatterns)
> +    WhiteListedSymbolRegexes.emplace_back(Pattern);
>  }
>
>  void ChangeNamespaceTool::registerMatchers(ast_matchers::MatchFinder
> *Finder) {
> @@ -736,6 +740,9 @@ void ChangeNamespaceTool::replaceQualifi
>            Result.SourceManager->getSpellingLoc(End)),
>        *Result.SourceManager, Result.Context->getLangOpts());
>    std::string FromDeclName = FromDecl->getQualifiedNameAsString();
> +  for (llvm::Regex &RE : WhiteListedSymbolRegexes)
> +    if (RE.match(FromDeclName))
> +      return;
>    std::string ReplaceName =
>        getShortestQualifiedNameInNamespace(FromDeclName, NewNs);
>    // Checks if there is any using namespace declarations that can shorten
> the
>
> Modified: clang-tools-extra/trunk/change-namespace/ChangeNamespace.h
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/change-namespace/ChangeNamespace.h?rev=296110&r1=296109&r2=296110&view=diff
>
> ==============================================================================
> --- clang-tools-extra/trunk/change-namespace/ChangeNamespace.h (original)
> +++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.h Fri Feb 24
> 05:54:45 2017
> @@ -50,6 +50,7 @@ public:
>    // files matching `FilePattern`.
>    ChangeNamespaceTool(
>        llvm::StringRef OldNs, llvm::StringRef NewNs, llvm::StringRef
> FilePattern,
> +      llvm::ArrayRef<std::string> WhiteListedSymbolPatterns,
>        std::map<std::string, tooling::Replacements> *FileToReplacements,
>        llvm::StringRef FallbackStyle = "LLVM");
>
> @@ -164,6 +165,9 @@ private:
>    // CallExpr and one as DeclRefExpr), we record all DeclRefExpr's that
> have
>    // been processed so that we don't handle them twice.
>    llvm::SmallPtrSet<const clang::DeclRefExpr*, 16> ProcessedFuncRefs;
> +  // Patterns of symbol names whose references are not expected to be
> updated
> +  // when changing namespaces around them.
> +  std::vector<llvm::Regex> WhiteListedSymbolRegexes;
>  };
>
>  } // namespace change_namespace
>
> Modified:
> clang-tools-extra/trunk/change-namespace/tool/ClangChangeNamespace.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/change-namespace/tool/ClangChangeNamespace.cpp?rev=296110&r1=296109&r2=296110&view=diff
>
> ==============================================================================
> --- clang-tools-extra/trunk/change-namespace/tool/ClangChangeNamespace.cpp
> (original)
> +++ clang-tools-extra/trunk/change-namespace/tool/ClangChangeNamespace.cpp
> Fri Feb 24 05:54:45 2017
> @@ -73,6 +73,25 @@ cl::opt<std::string> Style("style",
>                             cl::desc("The style name used for
> reformatting."),
>                             cl::init("LLVM"),
> cl::cat(ChangeNamespaceCategory));
>
> +cl::opt<std::string> WhiteListFile(
> +    "whitelist_file",
> +    cl::desc("A file containing regexes of symbol names that are not
> expected "
> +             "to be updated when changing namespaces around them."),
> +    cl::init(""), cl::cat(ChangeNamespaceCategory));
> +
> +llvm::ErrorOr<std::vector<std::string>> GetWhiteListedSymbolPatterns() {
> +  llvm::SmallVector<StringRef, 8> Lines;
> +  if (!WhiteListFile.empty()) {
> +    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> File =
> +        llvm::MemoryBuffer::getFile(WhiteListFile);
> +    if (!File)
> +      return File.getError();
> +    llvm::StringRef Content = File.get()->getBuffer();
> +    Content.split(Lines, '\n', /*MaxSplit=*/-1, /*KeepEmpty=*/false);
> +  }
> +  return std::vector<std::string>(Lines.begin(), Lines.end());
> +}
> +
>  } // anonymous namespace
>
>  int main(int argc, const char **argv) {
> @@ -81,8 +100,16 @@ int main(int argc, const char **argv) {
>                                               ChangeNamespaceCategory);
>    const auto &Files = OptionsParser.getSourcePathList();
>    tooling::RefactoringTool Tool(OptionsParser.getCompilations(), Files);
> +  llvm::ErrorOr<std::vector<std::string>> WhiteListPatterns =
> +      GetWhiteListedSymbolPatterns();
> +  if (!WhiteListPatterns) {
> +    llvm::errs() << "Failed to open whitelist file " << WhiteListFile <<
> ". "
> +                 << WhiteListPatterns.getError().message() << "\n";
> +    return 1;
> +  }
>    change_namespace::ChangeNamespaceTool NamespaceTool(
> -      OldNamespace, NewNamespace, FilePattern, &Tool.getReplacements(),
> Style);
> +      OldNamespace, NewNamespace, FilePattern, *WhiteListPatterns,
> +      &Tool.getReplacements(), Style);
>    ast_matchers::MatchFinder Finder;
>    NamespaceTool.registerMatchers(&Finder);
>    std::unique_ptr<tooling::FrontendActionFactory> Factory =
>
> Added: clang-tools-extra/trunk/test/change-namespace/Inputs/fake-std.h
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/change-namespace/Inputs/fake-std.h?rev=296110&view=auto
>
> ==============================================================================
> --- clang-tools-extra/trunk/test/change-namespace/Inputs/fake-std.h (added)
> +++ clang-tools-extra/trunk/test/change-namespace/Inputs/fake-std.h Fri
> Feb 24 05:54:45 2017
> @@ -0,0 +1,5 @@
> +namespace std {
> +  class STD {};
> +}
> +
> +using namespace std;
>
> Added: clang-tools-extra/trunk/test/change-namespace/white-list.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/change-namespace/white-list.cpp?rev=296110&view=auto
>
> ==============================================================================
> --- clang-tools-extra/trunk/test/change-namespace/white-list.cpp (added)
> +++ clang-tools-extra/trunk/test/change-namespace/white-list.cpp Fri Feb
> 24 05:54:45 2017
> @@ -0,0 +1,19 @@
> +// RUN: echo "^std::.*$" > %T/white-list.txt
> +// RUN: clang-change-namespace -old_namespace "na::nb" -new_namespace
> "x::y" --file_pattern ".*" --whitelist_file %T/white-list.txt %s -- | sed
> 's,// CHECK.*,,' | FileCheck %s
> +
> +#include "Inputs/fake-std.h"
> +
> +// CHECK: namespace x {
> +// CHECK-NEXT: namespace y {
> +namespace na {
> +namespace nb {
> +void f() {
> +  std::STD x1;
> +  STD x2;
> +// CHECK: {{^}}  std::STD x1;{{$}}
> +// CHECK-NEXT: {{^}}  STD x2;{{$}}
> +}
> +// CHECK: } // namespace y
> +// CHECK-NEXT: } // namespace x
> +}
> +}
>
> Modified:
> clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp?rev=296110&r1=296109&r2=296110&view=diff
>
> ==============================================================================
> ---
> clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
> (original)
> +++
> clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
> Fri Feb 24 05:54:45 2017
> @@ -38,7 +38,8 @@ public:
>
>      std::map<std::string, tooling::Replacements> FileToReplacements;
>      change_namespace::ChangeNamespaceTool NamespaceTool(
> -        OldNamespace, NewNamespace, FilePattern, &FileToReplacements);
> +        OldNamespace, NewNamespace, FilePattern,
> +        /*WhiteListedSymbolPatterns*/ {}, &FileToReplacements);
>      ast_matchers::MatchFinder Finder;
>      NamespaceTool.registerMatchers(&Finder);
>      std::unique_ptr<tooling::FrontendActionFactory> Factory =
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170228/663182c5/attachment-0001.html>


More information about the cfe-commits mailing list