[PATCH] D124918: [clang-tidy] Add a new check for non-trivial unused variables.

2022-06-03 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp:90
+if (!Op->isAssignmentOp()) {
+  markSideEffectFree(Op->getRHS());
+}

Perhaps I'm not understanding precisely what `markSideEffectFree` means, but 
this doesn't seem right to me:

```
int *data = ...;
auto getNextEntry = [&] () -> int& { return *++data; };
int n = (getNextDataEntry() + 17); // We can't mark RHS as side-effect free.
*data = n;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124918/new/

https://reviews.llvm.org/D124918

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

[PATCH] D126888: Add a parameter to LoadFromASTFile that accepts a file system and defaults to the real file-system.

2022-06-02 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer created this revision.
asoffer added a reviewer: ymandel.
Herald added a project: All.
asoffer requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126888

Files:
  clang/include/clang/Frontend/ASTUnit.h
  clang/lib/Frontend/ASTUnit.cpp


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -759,7 +759,8 @@
 WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
 const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
 bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
-bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
+bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile,
+IntrusiveRefCntPtr VFS) {
   std::unique_ptr AST(new ASTUnit(true));
 
   // Recover resources if we crash before exiting this method.
@@ -775,8 +776,6 @@
   AST->OnlyLocalDecls = OnlyLocalDecls;
   AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->Diagnostics = Diags;
-  IntrusiveRefCntPtr VFS =
-  llvm::vfs::getRealFileSystem();
   AST->FileMgr = new FileManager(FileSystemOpts, VFS);
   AST->UserFilesAreVolatile = UserFilesAreVolatile;
   AST->SourceMgr = new SourceManager(AST->getDiagnostics(),
Index: clang/include/clang/Frontend/ASTUnit.h
===
--- clang/include/clang/Frontend/ASTUnit.h
+++ clang/include/clang/Frontend/ASTUnit.h
@@ -696,7 +696,9 @@
   bool UseDebugInfo = false, bool OnlyLocalDecls = false,
   CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
   bool AllowASTWithCompilerErrors = false,
-  bool UserFilesAreVolatile = false);
+  bool UserFilesAreVolatile = false,
+  IntrusiveRefCntPtr VFS =
+  llvm::vfs::getRealFileSystem());
 
 private:
   /// Helper function for \c LoadFromCompilerInvocation() and


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -759,7 +759,8 @@
 WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
 const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
 bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
-bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
+bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile,
+IntrusiveRefCntPtr VFS) {
   std::unique_ptr AST(new ASTUnit(true));
 
   // Recover resources if we crash before exiting this method.
@@ -775,8 +776,6 @@
   AST->OnlyLocalDecls = OnlyLocalDecls;
   AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->Diagnostics = Diags;
-  IntrusiveRefCntPtr VFS =
-  llvm::vfs::getRealFileSystem();
   AST->FileMgr = new FileManager(FileSystemOpts, VFS);
   AST->UserFilesAreVolatile = UserFilesAreVolatile;
   AST->SourceMgr = new SourceManager(AST->getDiagnostics(),
Index: clang/include/clang/Frontend/ASTUnit.h
===
--- clang/include/clang/Frontend/ASTUnit.h
+++ clang/include/clang/Frontend/ASTUnit.h
@@ -696,7 +696,9 @@
   bool UseDebugInfo = false, bool OnlyLocalDecls = false,
   CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
   bool AllowASTWithCompilerErrors = false,
-  bool UserFilesAreVolatile = false);
+  bool UserFilesAreVolatile = false,
+  IntrusiveRefCntPtr VFS =
+  llvm::vfs::getRealFileSystem());
 
 private:
   /// Helper function for \c LoadFromCompilerInvocation() and
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126806: Add an overload to ASTUnit::LoadFromASTFile that accepts a virtual file system.

2022-06-02 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer abandoned this revision.
asoffer added a comment.

I don't know what happened here, but when I update a diff half my changes 
disappear. I'm going to recreate this in a new diff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126806/new/

https://reviews.llvm.org/D126806

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

[PATCH] D126806: Add an overload to ASTUnit::LoadFromASTFile that accepts a virtual file system.

2022-06-02 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 433747.
asoffer added a comment.

Use default arguments instead of overloads.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126806/new/

https://reviews.llvm.org/D126806

Files:
  clang/lib/Frontend/ASTUnit.cpp


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -866,18 +866,6 @@
   return AST;
 }
 
-std::unique_ptr ASTUnit::LoadFromASTFile(
-const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
-WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
-const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
-bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
-bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
-  return LoadFromASTFile(Filename, PCHContainerRdr, ToLoad, Diags,
- llvm::vfs::getRealFileSystem(), FileSystemOpts,
- UseDebugInfo, OnlyLocalDecls, CaptureDiagnostics,
- AllowASTWithCompilerErrors, UserFilesAreVolatile);
-}
-
 /// Add the given macro to the hash of all top-level entities.
 static void AddDefinedMacroToHash(const Token &MacroNameTok, unsigned &Hash) {
   Hash = llvm::djbHash(MacroNameTok.getIdentifierInfo()->getName(), Hash);


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -866,18 +866,6 @@
   return AST;
 }
 
-std::unique_ptr ASTUnit::LoadFromASTFile(
-const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
-WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
-const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
-bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
-bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
-  return LoadFromASTFile(Filename, PCHContainerRdr, ToLoad, Diags,
- llvm::vfs::getRealFileSystem(), FileSystemOpts,
- UseDebugInfo, OnlyLocalDecls, CaptureDiagnostics,
- AllowASTWithCompilerErrors, UserFilesAreVolatile);
-}
-
 /// Add the given macro to the hash of all top-level entities.
 static void AddDefinedMacroToHash(const Token &MacroNameTok, unsigned &Hash) {
   Hash = llvm::djbHash(MacroNameTok.getIdentifierInfo()->getName(), Hash);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126881: Use default arguments instead of overloads.

2022-06-02 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 433745.
asoffer added a comment.

Remove non-existant overload from cpp file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126881/new/

https://reviews.llvm.org/D126881

Files:
  clang/lib/Frontend/ASTUnit.cpp


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -866,18 +866,6 @@
   return AST;
 }
 
-std::unique_ptr ASTUnit::LoadFromASTFile(
-const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
-WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
-const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
-bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
-bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
-  return LoadFromASTFile(Filename, PCHContainerRdr, ToLoad, Diags,
- llvm::vfs::getRealFileSystem(), FileSystemOpts,
- UseDebugInfo, OnlyLocalDecls, CaptureDiagnostics,
- AllowASTWithCompilerErrors, UserFilesAreVolatile);
-}
-
 /// Add the given macro to the hash of all top-level entities.
 static void AddDefinedMacroToHash(const Token &MacroNameTok, unsigned &Hash) {
   Hash = llvm::djbHash(MacroNameTok.getIdentifierInfo()->getName(), Hash);


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -866,18 +866,6 @@
   return AST;
 }
 
-std::unique_ptr ASTUnit::LoadFromASTFile(
-const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
-WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
-const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
-bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
-bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
-  return LoadFromASTFile(Filename, PCHContainerRdr, ToLoad, Diags,
- llvm::vfs::getRealFileSystem(), FileSystemOpts,
- UseDebugInfo, OnlyLocalDecls, CaptureDiagnostics,
- AllowASTWithCompilerErrors, UserFilesAreVolatile);
-}
-
 /// Add the given macro to the hash of all top-level entities.
 static void AddDefinedMacroToHash(const Token &MacroNameTok, unsigned &Hash) {
   Hash = llvm::djbHash(MacroNameTok.getIdentifierInfo()->getName(), Hash);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126881: Use default arguments instead of overloads.

2022-06-02 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 433744.
asoffer added a comment.

Use default arguments instead of overloads.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126881/new/

https://reviews.llvm.org/D126881

Files:
  clang/include/clang/Frontend/ASTUnit.h
  clang/lib/Frontend/ASTUnit.cpp


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -757,6 +757,7 @@
 std::unique_ptr ASTUnit::LoadFromASTFile(
 const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
 WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+IntrusiveRefCntPtr VFS,
 const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
 bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
 bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
@@ -775,8 +776,6 @@
   AST->OnlyLocalDecls = OnlyLocalDecls;
   AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->Diagnostics = Diags;
-  IntrusiveRefCntPtr VFS =
-  llvm::vfs::getRealFileSystem();
   AST->FileMgr = new FileManager(FileSystemOpts, VFS);
   AST->UserFilesAreVolatile = UserFilesAreVolatile;
   AST->SourceMgr = new SourceManager(AST->getDiagnostics(),
@@ -867,6 +866,18 @@
   return AST;
 }
 
+std::unique_ptr ASTUnit::LoadFromASTFile(
+const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
+WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
+bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
+bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
+  return LoadFromASTFile(Filename, PCHContainerRdr, ToLoad, Diags,
+ llvm::vfs::getRealFileSystem(), FileSystemOpts,
+ UseDebugInfo, OnlyLocalDecls, CaptureDiagnostics,
+ AllowASTWithCompilerErrors, UserFilesAreVolatile);
+}
+
 /// Add the given macro to the hash of all top-level entities.
 static void AddDefinedMacroToHash(const Token &MacroNameTok, unsigned &Hash) {
   Hash = llvm::djbHash(MacroNameTok.getIdentifierInfo()->getName(), Hash);
Index: clang/include/clang/Frontend/ASTUnit.h
===
--- clang/include/clang/Frontend/ASTUnit.h
+++ clang/include/clang/Frontend/ASTUnit.h
@@ -688,17 +688,25 @@
   /// lifetime is expected to extend past that of the returned ASTUnit.
   ///
   /// \returns - The initialized ASTUnit or null if the AST failed to load.
-  static std::unique_ptr
-  LoadFromASTFile(const std::string &Filename,
-  const PCHContainerReader &PCHContainerRdr, WhatToLoad ToLoad,
-  IntrusiveRefCntPtr Diags,
-  const FileSystemOptions &FileSystemOpts,
-  bool UseDebugInfo = false, bool OnlyLocalDecls = false,
-  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
-  bool AllowASTWithCompilerErrors = false,
-  bool UserFilesAreVolatile = false);
+  static std::unique_ptr LoadFromASTFile(
+  const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
+  WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+  IntrusiveRefCntPtr VFS,
+  const FileSystemOptions &FileSystemOpts, bool UseDebugInfo = false,
+  bool OnlyLocalDecls = false,
+  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
+  bool AllowASTWithCompilerErrors = false,
+  bool UserFilesAreVolatile = false);
+  static std::unique_ptr LoadFromASTFile(
+  const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
+  WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+  const FileSystemOptions &FileSystemOpts, bool UseDebugInfo = false,
+  bool OnlyLocalDecls = false,
+  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
+  bool AllowASTWithCompilerErrors = false,
+  bool UserFilesAreVolatile = false);
 
-private:
+ private:
   /// Helper function for \c LoadFromCompilerInvocation() and
   /// \c LoadFromCommandLine(), which loads an AST from a compiler invocation.
   ///


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -757,6 +757,7 @@
 std::unique_ptr ASTUnit::LoadFromASTFile(
 const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
 WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+IntrusiveRefCntPtr VFS,
 const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
 bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
 bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
@@ -775,8 +776,6 @@
   AST->OnlyLocalDecls = OnlyLocalDecls;
   AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->Diagnostics = Diags;
-  IntrusiveRefCntPtr VFS =
-

[PATCH] D126881: Use default arguments instead of overloads.

2022-06-02 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer created this revision.
Herald added a project: All.
asoffer requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126881

Files:
  clang/include/clang/Frontend/ASTUnit.h
  clang/lib/Frontend/ASTUnit.cpp


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -757,6 +757,7 @@
 std::unique_ptr ASTUnit::LoadFromASTFile(
 const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
 WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+IntrusiveRefCntPtr VFS,
 const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
 bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
 bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
@@ -775,8 +776,6 @@
   AST->OnlyLocalDecls = OnlyLocalDecls;
   AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->Diagnostics = Diags;
-  IntrusiveRefCntPtr VFS =
-  llvm::vfs::getRealFileSystem();
   AST->FileMgr = new FileManager(FileSystemOpts, VFS);
   AST->UserFilesAreVolatile = UserFilesAreVolatile;
   AST->SourceMgr = new SourceManager(AST->getDiagnostics(),
@@ -867,6 +866,18 @@
   return AST;
 }
 
+std::unique_ptr ASTUnit::LoadFromASTFile(
+const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
+WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
+bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
+bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
+  return LoadFromASTFile(Filename, PCHContainerRdr, ToLoad, Diags,
+ llvm::vfs::getRealFileSystem(), FileSystemOpts,
+ UseDebugInfo, OnlyLocalDecls, CaptureDiagnostics,
+ AllowASTWithCompilerErrors, UserFilesAreVolatile);
+}
+
 /// Add the given macro to the hash of all top-level entities.
 static void AddDefinedMacroToHash(const Token &MacroNameTok, unsigned &Hash) {
   Hash = llvm::djbHash(MacroNameTok.getIdentifierInfo()->getName(), Hash);
Index: clang/include/clang/Frontend/ASTUnit.h
===
--- clang/include/clang/Frontend/ASTUnit.h
+++ clang/include/clang/Frontend/ASTUnit.h
@@ -688,17 +688,25 @@
   /// lifetime is expected to extend past that of the returned ASTUnit.
   ///
   /// \returns - The initialized ASTUnit or null if the AST failed to load.
-  static std::unique_ptr
-  LoadFromASTFile(const std::string &Filename,
-  const PCHContainerReader &PCHContainerRdr, WhatToLoad ToLoad,
-  IntrusiveRefCntPtr Diags,
-  const FileSystemOptions &FileSystemOpts,
-  bool UseDebugInfo = false, bool OnlyLocalDecls = false,
-  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
-  bool AllowASTWithCompilerErrors = false,
-  bool UserFilesAreVolatile = false);
+  static std::unique_ptr LoadFromASTFile(
+  const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
+  WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+  IntrusiveRefCntPtr VFS,
+  const FileSystemOptions &FileSystemOpts, bool UseDebugInfo = false,
+  bool OnlyLocalDecls = false,
+  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
+  bool AllowASTWithCompilerErrors = false,
+  bool UserFilesAreVolatile = false);
+  static std::unique_ptr LoadFromASTFile(
+  const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
+  WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+  const FileSystemOptions &FileSystemOpts, bool UseDebugInfo = false,
+  bool OnlyLocalDecls = false,
+  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
+  bool AllowASTWithCompilerErrors = false,
+  bool UserFilesAreVolatile = false);
 
-private:
+ private:
   /// Helper function for \c LoadFromCompilerInvocation() and
   /// \c LoadFromCommandLine(), which loads an AST from a compiler invocation.
   ///


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -757,6 +757,7 @@
 std::unique_ptr ASTUnit::LoadFromASTFile(
 const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
 WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+IntrusiveRefCntPtr VFS,
 const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
 bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
 bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
@@ -775,8 +776,6 @@
   AST->OnlyLocalDecls = OnlyLocalDecls;
   AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->Diagnostics = Diags;
-  IntrusiveRefCntPtr VFS =
-  llvm::

[PATCH] D126806: Add an overload to ASTUnit::LoadFromASTFile that accepts a virtual file system.

2022-06-01 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer created this revision.
Herald added a project: All.
asoffer requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126806

Files:
  clang/include/clang/Frontend/ASTUnit.h
  clang/lib/Frontend/ASTUnit.cpp


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -757,6 +757,7 @@
 std::unique_ptr ASTUnit::LoadFromASTFile(
 const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
 WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+IntrusiveRefCntPtr VFS,
 const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
 bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
 bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
@@ -775,8 +776,6 @@
   AST->OnlyLocalDecls = OnlyLocalDecls;
   AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->Diagnostics = Diags;
-  IntrusiveRefCntPtr VFS =
-  llvm::vfs::getRealFileSystem();
   AST->FileMgr = new FileManager(FileSystemOpts, VFS);
   AST->UserFilesAreVolatile = UserFilesAreVolatile;
   AST->SourceMgr = new SourceManager(AST->getDiagnostics(),
@@ -867,6 +866,18 @@
   return AST;
 }
 
+std::unique_ptr ASTUnit::LoadFromASTFile(
+const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
+WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
+bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
+bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
+  return LoadFromASTFile(Filename, PCHContainerRdr, ToLoad, Diags,
+ llvm::vfs::getRealFileSystem(), FileSystemOpts,
+ UseDebugInfo, OnlyLocalDecls, CaptureDiagnostics,
+ AllowASTWithCompilerErrors, UserFilesAreVolatile);
+}
+
 /// Add the given macro to the hash of all top-level entities.
 static void AddDefinedMacroToHash(const Token &MacroNameTok, unsigned &Hash) {
   Hash = llvm::djbHash(MacroNameTok.getIdentifierInfo()->getName(), Hash);
Index: clang/include/clang/Frontend/ASTUnit.h
===
--- clang/include/clang/Frontend/ASTUnit.h
+++ clang/include/clang/Frontend/ASTUnit.h
@@ -688,17 +688,25 @@
   /// lifetime is expected to extend past that of the returned ASTUnit.
   ///
   /// \returns - The initialized ASTUnit or null if the AST failed to load.
-  static std::unique_ptr
-  LoadFromASTFile(const std::string &Filename,
-  const PCHContainerReader &PCHContainerRdr, WhatToLoad ToLoad,
-  IntrusiveRefCntPtr Diags,
-  const FileSystemOptions &FileSystemOpts,
-  bool UseDebugInfo = false, bool OnlyLocalDecls = false,
-  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
-  bool AllowASTWithCompilerErrors = false,
-  bool UserFilesAreVolatile = false);
+  static std::unique_ptr LoadFromASTFile(
+  const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
+  WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+  IntrusiveRefCntPtr VFS,
+  const FileSystemOptions &FileSystemOpts, bool UseDebugInfo = false,
+  bool OnlyLocalDecls = false,
+  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
+  bool AllowASTWithCompilerErrors = false,
+  bool UserFilesAreVolatile = false);
+  static std::unique_ptr LoadFromASTFile(
+  const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
+  WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+  const FileSystemOptions &FileSystemOpts, bool UseDebugInfo = false,
+  bool OnlyLocalDecls = false,
+  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
+  bool AllowASTWithCompilerErrors = false,
+  bool UserFilesAreVolatile = false);
 
-private:
+ private:
   /// Helper function for \c LoadFromCompilerInvocation() and
   /// \c LoadFromCommandLine(), which loads an AST from a compiler invocation.
   ///


Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -757,6 +757,7 @@
 std::unique_ptr ASTUnit::LoadFromASTFile(
 const std::string &Filename, const PCHContainerReader &PCHContainerRdr,
 WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
+IntrusiveRefCntPtr VFS,
 const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
 bool OnlyLocalDecls, CaptureDiagsKind CaptureDiagnostics,
 bool AllowASTWithCompilerErrors, bool UserFilesAreVolatile) {
@@ -775,8 +776,6 @@
   AST->OnlyLocalDecls = OnlyLocalDecls;
   AST->CaptureDiagnostics = CaptureDiagnostics;
   AST->Diagnostics = Diags;
-  IntrusiveRefCntPtr VFS =
-  llvm::

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

2022-04-25 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer added a comment.

In D122920#3471865 , @yihanaa wrote:

> In D122920#3471654 , @erichkeane 
> wrote:
>
>> @yihanaa : I'd suggest seeing the conversation that @rsmith @aaron.ballman 
>> and I are having about this builtin here: https://reviews.llvm.org/D124221
>>
>> In general it looks like the three of us think that this builtin needs an 
>> overhaul in implementation/functionality in order to be further useful. 
>> While we very much appreciate your attempts to try to improve it, we believe 
>> there needs to be larger-scale changes.
>
> Thanks for your reply, I would like to get involved and help if needed

While I eagerly await `__builtin_reflect_struct`, this change still provides 
significant value in fixing a regression with `__builtin_dump_struct`. Should 
we proceed with this change and that proposal independently?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

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

[PATCH] D120360: [libTooling] Generalize string explanation as templated metadata

2022-03-03 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer added inline comments.



Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:103-108
+  template >>::value,
+int> = 0>

Given that we're simply passing this off to the NoMetadataImpl which accepts a 
std::function, I don't see any reason to accept a generic parameter via sfinae. 
We could just accept the std::function and move it. Then the implicit 
conversion from whatever is passed in to std::function will do the heavy sfinae 
lifting.



Comment at: clang/include/clang/Tooling/Transformer/Transformer.h:116
+  explicit Transformer(transformer::RewriteRuleWith Rule,
+   ConsumerFn Consumer);
 

li.zhe.hua wrote:
> ymandel wrote:
> > why won't we have the same SFINAE here to ensure ConsumerFn is invocable?
> I can't figure out how to implement the SFINAE without instantiating 
> `Result` (which is invalid because `T Metadata` is illegal then). My 
> current attempt is
> 
> ```
>   template <
>   typename T, typename ConsumerFn,
>   std::enable_if_t<
>   std::conjunction<
>   std::negation>,
>   llvm::is_invocable llvm::Expected>>>::value,
>   int> = 0>
>   explicit Transformer(transformer::RewriteRuleWith Rule,
>ConsumerFn Consumer);
> ```
> 
> I suppose I could provide a specialization of `Result` that is valid? I 
> guess I'll go with that, and just document why it exists?
Accepting the std:function directly should simplify this and avoid the need for 
the Result specialization.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120360/new/

https://reviews.llvm.org/D120360

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

[PATCH] D116377: [libTooling] Adds more support for constructing object access expressions.

2022-01-07 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer added inline comments.



Comment at: clang/lib/Tooling/Transformer/SourceCodeBuilders.cpp:73
+  cxxRecordDecl(hasAnyName("::std::unique_ptr", "::std::shared_ptr"));
+  const auto QuacksLikeASmartPointer = cxxRecordDecl(
+  hasMethod(cxxMethodDecl(hasOverloadedOperatorName("->"),

Naming nit: This could be confusing for anyone not familiar with the "duck 
typing" idiom.
BehavesLikeASmartPointer?



Comment at: clang/lib/Tooling/Transformer/SourceCodeBuilders.cpp:78
+  returns(qualType(references(type()));
+  const auto SmartPointer = qualType(hasDeclaration(
+  cxxRecordDecl(anyOf(KnownSmartPointer, QuacksLikeASmartPointer;

I think std::optional satisfies these requirements but should not be considered 
a smart-pointer.



Comment at: clang/lib/Tooling/Transformer/SourceCodeBuilders.cpp:79
+  const auto SmartPointer = qualType(hasDeclaration(
+  cxxRecordDecl(anyOf(KnownSmartPointer, QuacksLikeASmartPointer;
+  return match(SmartPointer, Ty, Context).size() > 0;

The known smart pointers quack like smart pointers, so this is redundant, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116377/new/

https://reviews.llvm.org/D116377

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

[PATCH] D84310: [libTooling] Add assorted `EditGenerator` combinators.

2020-07-23 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer accepted this revision.
asoffer added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:125
+/// not bound, then no edits are produced.
+inline EditGenerator ifBound(std::string ID, ASTEdit TrueEdit) {
+  return ifBound(std::move(ID), edit(std::move(TrueEdit)), noEdits());

I don't know about LLVM style preferences here, but a default argument to the 
example above seems reasonable too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84310/new/

https://reviews.llvm.org/D84310



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

[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.

2020-07-21 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 279572.
asoffer added a comment.

Construct types for default metadata explicitly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83820/new/

https://reviews.llvm.org/D83820

Files:
  clang/include/clang/Tooling/Transformer/RewriteRule.h
  clang/lib/Tooling/Transformer/RewriteRule.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -440,6 +440,12 @@
 }
 
 TEST_F(TransformerTest, WithMetadata) {
+  auto makeMetadata = [](const MatchFinder::MatchResult &R) -> llvm::Any {
+int N =
+R.Nodes.getNodeAs("int")->getValue().getLimitedValue();
+return N;
+  };
+
   std::string Input = R"cc(
 int f() {
   int x = 5;
@@ -448,8 +454,11 @@
   )cc";
 
   Transformer T(
-  makeRule(declStmt().bind("decl"),
-   withMetadata(remove(statement(std::string("decl"))), 17)),
+  makeRule(
+  declStmt(containsDeclaration(0, varDecl(hasInitializer(
+  integerLiteral().bind("int")
+  .bind("decl"),
+  withMetadata(remove(statement(std::string("decl"))), makeMetadata)),
   consumer());
   T.registerMatchers(&MatchFinder);
   auto Factory = newFrontendActionFactory(&MatchFinder);
@@ -459,7 +468,7 @@
   ASSERT_EQ(Changes.size(), 1u);
   const llvm::Any &Metadata = Changes[0].getMetadata();
   ASSERT_TRUE(llvm::any_isa(Metadata));
-  EXPECT_THAT(llvm::any_cast(Metadata), 17);
+  EXPECT_THAT(llvm::any_cast(Metadata), 5);
 }
 
 TEST_F(TransformerTest, MultiChange) {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -44,10 +44,13 @@
 auto Replacement = E.Replacement->eval(Result);
 if (!Replacement)
   return Replacement.takeError();
+auto Metadata = E.Metadata(Result);
+if (!Metadata)
+  return Metadata.takeError();
 transformer::Edit T;
 T.Range = *EditRange;
 T.Replacement = std::move(*Replacement);
-T.Metadata = E.Metadata;
+T.Metadata = std::move(*Metadata);
 Edits.push_back(std::move(T));
   }
   return Edits;
Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -47,6 +47,8 @@
 
 using TextGenerator = std::shared_ptr>;
 
+using AnyGenerator = MatchConsumer;
+
 // Description of a source-code edit, expressed in terms of an AST node.
 // Includes: an ID for the (bound) node, a selector for source related to the
 // node, a replacement and, optionally, an explanation for the edit.
@@ -87,7 +89,12 @@
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
-  llvm::Any Metadata;
+  // Not all transformations will want or need to attach metadata and therefore
+  // should not be requierd to do so.
+  AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &)
+  -> llvm::Expected {
+return llvm::Expected(llvm::Any());
+  };
 };
 
 /// Lifts a list of `ASTEdit`s into an `EditGenerator`.
@@ -261,9 +268,27 @@
 /// Removes the source selected by \p S.
 ASTEdit remove(RangeSelector S);
 
-inline ASTEdit withMetadata(ASTEdit edit, llvm::Any Metadata) {
-  edit.Metadata = std::move(Metadata);
-  return edit;
+// FIXME: If `Metadata` returns an `llvm::Expected` the `AnyGenerator` will
+// construct an `llvm::Expected` where no error is present but the
+// `llvm::Any` holds the error. This is unlikely but potentially surprising.
+// Perhaps the `llvm::Expected` should be unwrapped, or perhaps this should be a
+// compile-time error. No solution here is perfect.
+//
+// Note: This function template accepts any type callable with a MatchResult
+// rather than a `std::function` because the return-type needs to be deduced. If
+// it accepted a `std::function`, lambdas or other callable types
+// would not be able to deduce `R`, and users would be forced to specify
+// explicitly the type they intended to return by wrapping the lambda at the
+// call-site.
+template 
+inline ASTEdit withMetadata(ASTEdit Edit, Callable Metadata) {
+  Edit.Metadata =
+  [Gen = std::move(Metadata)](
+  const ast_matchers::MatchFinder::MatchResult &R) -> llvm::Any {
+return Gen(R);
+  };
+
+  return Edit;
 }
 
 /// The following three functions are a low-level part of the RewriteRule
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.

2020-07-16 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 278611.
asoffer marked an inline comment as done.
asoffer added a comment.

Typo fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83820/new/

https://reviews.llvm.org/D83820

Files:
  clang/include/clang/Tooling/Transformer/RewriteRule.h
  clang/lib/Tooling/Transformer/RewriteRule.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -440,6 +440,12 @@
 }
 
 TEST_F(TransformerTest, WithMetadata) {
+  auto makeMetadata = [](const MatchFinder::MatchResult &R) -> llvm::Any {
+int N =
+R.Nodes.getNodeAs("int")->getValue().getLimitedValue();
+return N;
+  };
+
   std::string Input = R"cc(
 int f() {
   int x = 5;
@@ -448,8 +454,11 @@
   )cc";
 
   Transformer T(
-  makeRule(declStmt().bind("decl"),
-   withMetadata(remove(statement(std::string("decl"))), 17)),
+  makeRule(
+  declStmt(containsDeclaration(0, varDecl(hasInitializer(
+  integerLiteral().bind("int")
+  .bind("decl"),
+  withMetadata(remove(statement(std::string("decl"))), makeMetadata)),
   consumer());
   T.registerMatchers(&MatchFinder);
   auto Factory = newFrontendActionFactory(&MatchFinder);
@@ -459,7 +468,7 @@
   ASSERT_EQ(Changes.size(), 1u);
   const llvm::Any &Metadata = Changes[0].getMetadata();
   ASSERT_TRUE(llvm::any_isa(Metadata));
-  EXPECT_THAT(llvm::any_cast(Metadata), 17);
+  EXPECT_THAT(llvm::any_cast(Metadata), 5);
 }
 
 TEST_F(TransformerTest, MultiChange) {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -44,10 +44,13 @@
 auto Replacement = E.Replacement->eval(Result);
 if (!Replacement)
   return Replacement.takeError();
+auto Metadata = E.Metadata(Result);
+if (!Metadata)
+  return Metadata.takeError();
 transformer::Edit T;
 T.Range = *EditRange;
 T.Replacement = std::move(*Replacement);
-T.Metadata = E.Metadata;
+T.Metadata = std::move(*Metadata);
 Edits.push_back(std::move(T));
   }
   return Edits;
Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -47,6 +47,8 @@
 
 using TextGenerator = std::shared_ptr>;
 
+using AnyGenerator = MatchConsumer;
+
 // Description of a source-code edit, expressed in terms of an AST node.
 // Includes: an ID for the (bound) node, a selector for source related to the
 // node, a replacement and, optionally, an explanation for the edit.
@@ -87,7 +89,11 @@
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
-  llvm::Any Metadata;
+  // Not all transformations will want or need to attach metadata and therefore
+  // should not be required to do so.
+  AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &) {
+return llvm::Any();
+  };
 };
 
 /// Lifts a list of `ASTEdit`s into an `EditGenerator`.
@@ -261,9 +267,27 @@
 /// Removes the source selected by \p S.
 ASTEdit remove(RangeSelector S);
 
-inline ASTEdit withMetadata(ASTEdit edit, llvm::Any Metadata) {
-  edit.Metadata = std::move(Metadata);
-  return edit;
+// FIXME: If `Metadata` returns an `llvm::Expected` the `AnyGenerator` will
+// construct an `llvm::Expected` where no error is present but the
+// `llvm::Any` holds the error. This is unlikely but potentially surprising.
+// Perhaps the `llvm::Expected` should be unwrapped, or perhaps this should be a
+// compile-time error. No solution here is perfect.
+//
+// Note: This function template accepts any type callable with a MatchResult
+// rather than a `std::function` because the return-type needs to be deduced. If
+// it accepted a `std::function`, lambdas or other callable
+// types would not be able to deduce `R`, and users would be forced to specify
+// explicitly the type they intended to return by wrapping the lambda at the
+// call-site.
+template 
+inline ASTEdit withMetadata(ASTEdit Edit, Callable Metadata) {
+  Edit.Metadata =
+  [Gen = std::move(Metadata)](
+  const ast_matchers::MatchFinder::MatchResult &R) -> llvm::Any {
+return Gen(R);
+  };
+
+  return Edit;
 }
 
 /// The following three functions are a low-level part of the RewriteRule
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.

2020-07-16 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer marked an inline comment as done.
asoffer added inline comments.



Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:93
+  // Not all transformations will want or need to attach metadata and therefore
+  // sholud not be requierd to do so.
   AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &) {

ymandel wrote:
> asoffer wrote:
> > ymandel wrote:
> > > nit: typos
> > > 
> > > The same applies to `Note`. Since this is a nullable type, can we ask 
> > > that the user check `Metadata != nullptr`?
> > I don't think I'm understanding the question. Where/when would users check 
> > for Metadata != nullptr?
> > 
> > Currently in this diff, any library construction of the Metadata field will 
> > be non-null. Users would have to explicitly pass null in if they wanted it 
> > to be null which would pretty quickly break when the generator is called, 
> > so this seems like a not-too-big foot-gun? Does that answer the question?
> Sorry, I meant code that consumes this struct. Mostly that's just 
> `translateEdits`. I realize now that `Note` is not a great example, since we 
> totally ignore it. However, it is intended as optional, so if we had the 
> code, it would be checking it for null first. Conversely, `TargetRange` and 
> `Replacement` are assumed to never be null, yet we don't set them to default 
> values here.  So, I think the first step is to decide if this is optional, 
> and (if not) the second is to consistently handle non-optional values in this 
> struct.
> 
> So, your code below would become something like:
> ```
> llvm::Any Metadata;
> if (E.Metadata != nullptr) {
>   if (auto M = E.Metadata(Result))
> Metadata = std::move(*M);
>   else
> return M.takeError();
> }
> ```
I don't see any particular reason to allow this field to take on it's null 
value. A null check could essentially avoid attaching metadata, but that's also 
what the default lambda would do here (because Any is also a nullable type).

That being said, sometimes I'm a bit UB-trigger-happy.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83820/new/

https://reviews.llvm.org/D83820



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

[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.

2020-07-16 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 278552.
asoffer added a comment.

Fix formatting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83820/new/

https://reviews.llvm.org/D83820

Files:
  clang/include/clang/Tooling/Transformer/RewriteRule.h
  clang/lib/Tooling/Transformer/RewriteRule.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -440,6 +440,12 @@
 }
 
 TEST_F(TransformerTest, WithMetadata) {
+  auto makeMetadata = [](const MatchFinder::MatchResult &R) -> llvm::Any {
+int N =
+R.Nodes.getNodeAs("int")->getValue().getLimitedValue();
+return N;
+  };
+
   std::string Input = R"cc(
 int f() {
   int x = 5;
@@ -448,8 +454,11 @@
   )cc";
 
   Transformer T(
-  makeRule(declStmt().bind("decl"),
-   withMetadata(remove(statement(std::string("decl"))), 17)),
+  makeRule(
+  declStmt(containsDeclaration(0, varDecl(hasInitializer(
+  integerLiteral().bind("int")
+  .bind("decl"),
+  withMetadata(remove(statement(std::string("decl"))), makeMetadata)),
   consumer());
   T.registerMatchers(&MatchFinder);
   auto Factory = newFrontendActionFactory(&MatchFinder);
@@ -459,7 +468,7 @@
   ASSERT_EQ(Changes.size(), 1u);
   const llvm::Any &Metadata = Changes[0].getMetadata();
   ASSERT_TRUE(llvm::any_isa(Metadata));
-  EXPECT_THAT(llvm::any_cast(Metadata), 17);
+  EXPECT_THAT(llvm::any_cast(Metadata), 5);
 }
 
 TEST_F(TransformerTest, MultiChange) {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -44,10 +44,13 @@
 auto Replacement = E.Replacement->eval(Result);
 if (!Replacement)
   return Replacement.takeError();
+auto Metadata = E.Metadata(Result);
+if (!Metadata)
+  return Metadata.takeError();
 transformer::Edit T;
 T.Range = *EditRange;
 T.Replacement = std::move(*Replacement);
-T.Metadata = E.Metadata;
+T.Metadata = std::move(*Metadata);
 Edits.push_back(std::move(T));
   }
   return Edits;
Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -47,6 +47,8 @@
 
 using TextGenerator = std::shared_ptr>;
 
+using AnyGenerator = MatchConsumer;
+
 // Description of a source-code edit, expressed in terms of an AST node.
 // Includes: an ID for the (bound) node, a selector for source related to the
 // node, a replacement and, optionally, an explanation for the edit.
@@ -87,7 +89,11 @@
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
-  llvm::Any Metadata;
+  // Not all transformations will want or need to attach metadata and therefore
+  // should not be requierd to do so.
+  AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &) {
+return llvm::Any();
+  };
 };
 
 /// Lifts a list of `ASTEdit`s into an `EditGenerator`.
@@ -261,9 +267,27 @@
 /// Removes the source selected by \p S.
 ASTEdit remove(RangeSelector S);
 
-inline ASTEdit withMetadata(ASTEdit edit, llvm::Any Metadata) {
-  edit.Metadata = std::move(Metadata);
-  return edit;
+// FIXME: If `Metadata` returns an `llvm::Expected` the `AnyGenerator` will
+// construct an `llvm::Expected` where no error is present but the
+// `llvm::Any` holds the error. This is unlikely but potentially surprising.
+// Perhaps the `llvm::Expected` should be unwrapped, or perhaps this should be a
+// compile-time error. No solution here is perfect.
+//
+// Note: This function template accepts any type callable with a MatchResult
+// rather than a `std::function` because the return-type needs to be deduced. If
+// it accepted a `std::function`, lambdas or other callable
+// types would not be able to deduce `R`, and users would be forced to specify
+// explicitly the type they intended to return by wrapping the lambda at the
+// call-site.
+template 
+inline ASTEdit withMetadata(ASTEdit Edit, Callable Metadata) {
+  Edit.Metadata =
+  [Gen = std::move(Metadata)](
+  const ast_matchers::MatchFinder::MatchResult &R) -> llvm::Any {
+return Gen(R);
+  };
+
+  return Edit;
 }
 
 /// The following three functions are a low-level part of the RewriteRule
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.

2020-07-15 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer added inline comments.



Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:93
+  // Not all transformations will want or need to attach metadata and therefore
+  // sholud not be requierd to do so.
   AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &) {

ymandel wrote:
> nit: typos
> 
> The same applies to `Note`. Since this is a nullable type, can we ask that 
> the user check `Metadata != nullptr`?
I don't think I'm understanding the question. Where/when would users check for 
Metadata != nullptr?

Currently in this diff, any library construction of the Metadata field will be 
non-null. Users would have to explicitly pass null in if they wanted it to be 
null which would pretty quickly break when the generator is called, so this 
seems like a not-too-big foot-gun? Does that answer the question?



Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:270
 
-// TODO(asoffer): If this returns an llvm::Expected, should we unwrap?
+// FIXME: If `Metadata` returns an `llvm::Expected` the `AnyGenerator` will
+// construct an `llvm::Expected` where no error is present but the

ymandel wrote:
> I think I understand now. Is the idea that we'll only get one implicit 
> construction from the compiler, so we want to use that "free" conversion on 
> the `llvm::Any`, rather than on the `llvm::Expected`, which would force the 
> user to explicitly construct the `llvm::Any`?
> 
> I think we should address this with separate functions (or overloads at 
> least), one for the case of never-fail metadata constructors and one for 
> failable constructors. That said, any computation that takes the matchresult 
> is prone to failure because of unbound nodes.  so, I would expect it to be 
> common for the Callable to be failable.  however, if you feel that's the 
> uncommon case, let's optimize towards the common case, like you've done.
> 
> With all that said, I agree that if the Callable returns an `Expected` we 
> should rewrap correctly, not bury in `Any`.
I think you're asking about why the generic callable instead of a std::function 
or LLVM equivalent type. It's not about implicit conversions but rather about 
deduction. Type deduction allows only for conversions on qualifiers (T to const 
T&, for example). So if we specified a std::function, it would either require:
1. The cannot pass lambdas without explicitly wrapping them in a std::function 
construction so that the return type can be deduced.
2. We have an explicit std::function, but then the 
user-provided callable would have to return an llvm::Any explicitly.
Neither of these seem ideal.

The Expected-unwrapping requires some ugly SFINAE, so I'd prefer to punt on 
this until we feel the need to have Expected's in metadata.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83820/new/

https://reviews.llvm.org/D83820



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

[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.

2020-07-15 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 278315.
asoffer marked 2 inline comments as done.
asoffer added a comment.

Typo fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83820/new/

https://reviews.llvm.org/D83820

Files:
  clang/include/clang/Tooling/Transformer/RewriteRule.h
  clang/lib/Tooling/Transformer/RewriteRule.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -440,6 +440,12 @@
 }
 
 TEST_F(TransformerTest, WithMetadata) {
+  auto makeMetadata = [](const MatchFinder::MatchResult &R) -> llvm::Any {
+int N =
+R.Nodes.getNodeAs("int")->getValue().getLimitedValue();
+return N;
+  };
+
   std::string Input = R"cc(
 int f() {
   int x = 5;
@@ -448,8 +454,11 @@
   )cc";
 
   Transformer T(
-  makeRule(declStmt().bind("decl"),
-   withMetadata(remove(statement(std::string("decl"))), 17)),
+  makeRule(
+  declStmt(containsDeclaration(0, varDecl(hasInitializer(
+  integerLiteral().bind("int")
+  .bind("decl"),
+  withMetadata(remove(statement(std::string("decl"))), makeMetadata)),
   consumer());
   T.registerMatchers(&MatchFinder);
   auto Factory = newFrontendActionFactory(&MatchFinder);
@@ -459,7 +468,7 @@
   ASSERT_EQ(Changes.size(), 1u);
   const llvm::Any &Metadata = Changes[0].getMetadata();
   ASSERT_TRUE(llvm::any_isa(Metadata));
-  EXPECT_THAT(llvm::any_cast(Metadata), 17);
+  EXPECT_THAT(llvm::any_cast(Metadata), 5);
 }
 
 TEST_F(TransformerTest, MultiChange) {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -44,10 +44,13 @@
 auto Replacement = E.Replacement->eval(Result);
 if (!Replacement)
   return Replacement.takeError();
+auto Metadata = E.Metadata(Result);
+if (!Metadata)
+  return Metadata.takeError();
 transformer::Edit T;
 T.Range = *EditRange;
 T.Replacement = std::move(*Replacement);
-T.Metadata = E.Metadata;
+T.Metadata = std::move(*Metadata);
 Edits.push_back(std::move(T));
   }
   return Edits;
Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -47,6 +47,8 @@
 
 using TextGenerator = std::shared_ptr>;
 
+using AnyGenerator = MatchConsumer;
+
 // Description of a source-code edit, expressed in terms of an AST node.
 // Includes: an ID for the (bound) node, a selector for source related to the
 // node, a replacement and, optionally, an explanation for the edit.
@@ -87,7 +89,11 @@
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
-  llvm::Any Metadata;
+  // Not all transformations will want or need to attach metadata and therefore
+  // should not be requierd to do so.
+  AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &) {
+return llvm::Any();
+  };
 };
 
 /// Lifts a list of `ASTEdit`s into an `EditGenerator`.
@@ -261,9 +267,27 @@
 /// Removes the source selected by \p S.
 ASTEdit remove(RangeSelector S);
 
-inline ASTEdit withMetadata(ASTEdit edit, llvm::Any Metadata) {
-  edit.Metadata = std::move(Metadata);
-  return edit;
+// FIXME: If `Metadata` returns an `llvm::Expected` the `AnyGenerator` will
+// construct an `llvm::Expected` where no error is present but the
+// `llvm::Any` holds the error. This is unlikely but potentially surprising.
+// Perhaps the `llvm::Expected` should be unwrapped, or perhaps this should be a
+// compile-time error. No solution here is perfect.
+//
+// Note: This function template accepts any type callable with a MatchResult
+// rather than a `std::function` because the return-type needs to be deduced. If
+// it accepted a `std::function`, lambdas or other callable types
+// would not be able to deduce `R`, and users would be forced to specify
+// explicitly the type they intended to return by wrapping the lambda at the
+// call-site.
+template 
+inline ASTEdit withMetadata(ASTEdit Edit, Callable Metadata) {
+  Edit.Metadata =
+  [Gen = std::move(Metadata)](
+  const ast_matchers::MatchFinder::MatchResult &R) -> llvm::Any {
+return Gen(R);
+  };
+
+  return Edit;
 }
 
 /// The following three functions are a low-level part of the RewriteRule
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.

2020-07-15 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer marked 4 inline comments as done.
asoffer added inline comments.



Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:92
   TextGenerator Note;
-  llvm::Any Metadata;
+  AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &) {
+return llvm::Any();

ymandel wrote:
> Why default, especially when we don't default the others? Is it worth a 
> comment?
Added comments.



Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:268
 
-inline ASTEdit withMetadata(ASTEdit edit, llvm::Any Metadata) {
-  edit.Metadata = std::move(Metadata);
-  return edit;
+// TODO(asoffer): If this returns an llvm::Expected, should we unwrap?
+template 

gribozavr2 wrote:
> What is "this" who is "we"? Should this be an implementation comment instead 
> (in the function body next to the relevant line)?
Added comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83820/new/

https://reviews.llvm.org/D83820



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

[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.

2020-07-15 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 278262.
asoffer added a comment.

Add comments describing why we provide defaults for Metadata generation and 
design of withMetadata function template.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83820/new/

https://reviews.llvm.org/D83820

Files:
  clang/include/clang/Tooling/Transformer/RewriteRule.h


Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -89,6 +89,8 @@
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
+  // Not all transformations will want or need to attach metadata and therefore
+  // sholud not be requierd to do so.
   AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &) {
 return llvm::Any();
   };
@@ -265,7 +267,18 @@
 /// Removes the source selected by \p S.
 ASTEdit remove(RangeSelector S);
 
-// TODO(asoffer): If this returns an llvm::Expected, should we unwrap?
+// FIXME: If `Metadata` returns an `llvm::Expected` the `AnyGenerator` will
+// construct an `llvm::Expected` where no error is present but the
+// `llvm::Any` holds the error. This is unlikely but potentially surprising.
+// Perhaps the `llvm::Expected` should be unwrapped, or perhaps this should be 
a
+// compile-time error. No solution here is perfect.
+//
+// Note: This function template accepts any type callable with a MatchResult
+// rather than a `std::function` because the return-type needs to be deduced. 
If
+// it accepted a `std::function`, lambdas or other callable 
types
+// would not be able to deduce `R`, and users would be forced to specify
+// explicitly the type they intended to return by wrapping the lambda at the
+// call-site.
 template 
 inline ASTEdit withMetadata(ASTEdit Edit, Callable Metadata) {
   Edit.Metadata =


Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -89,6 +89,8 @@
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
+  // Not all transformations will want or need to attach metadata and therefore
+  // sholud not be requierd to do so.
   AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &) {
 return llvm::Any();
   };
@@ -265,7 +267,18 @@
 /// Removes the source selected by \p S.
 ASTEdit remove(RangeSelector S);
 
-// TODO(asoffer): If this returns an llvm::Expected, should we unwrap?
+// FIXME: If `Metadata` returns an `llvm::Expected` the `AnyGenerator` will
+// construct an `llvm::Expected` where no error is present but the
+// `llvm::Any` holds the error. This is unlikely but potentially surprising.
+// Perhaps the `llvm::Expected` should be unwrapped, or perhaps this should be a
+// compile-time error. No solution here is perfect.
+//
+// Note: This function template accepts any type callable with a MatchResult
+// rather than a `std::function` because the return-type needs to be deduced. If
+// it accepted a `std::function`, lambdas or other callable types
+// would not be able to deduce `R`, and users would be forced to specify
+// explicitly the type they intended to return by wrapping the lambda at the
+// call-site.
 template 
 inline ASTEdit withMetadata(ASTEdit Edit, Callable Metadata) {
   Edit.Metadata =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.

2020-07-14 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer created this revision.
asoffer added reviewers: gribozavr, ymandel.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Metadata is being changed from an llvm::Any to a MatchConsumer so 
that it's evaluation can be be dependent on on MatchResults passed in.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83820

Files:
  clang/include/clang/Tooling/Transformer/RewriteRule.h
  clang/lib/Tooling/Transformer/RewriteRule.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -440,6 +440,12 @@
 }
 
 TEST_F(TransformerTest, WithMetadata) {
+  auto makeMetadata = [](const MatchFinder::MatchResult &R) -> llvm::Any {
+int N =
+R.Nodes.getNodeAs("int")->getValue().getLimitedValue();
+return N;
+  };
+
   std::string Input = R"cc(
 int f() {
   int x = 5;
@@ -448,8 +454,11 @@
   )cc";
 
   Transformer T(
-  makeRule(declStmt().bind("decl"),
-   withMetadata(remove(statement(std::string("decl"))), 17)),
+  makeRule(
+  declStmt(containsDeclaration(0, varDecl(hasInitializer(
+  integerLiteral().bind("int")
+  .bind("decl"),
+  withMetadata(remove(statement(std::string("decl"))), makeMetadata)),
   consumer());
   T.registerMatchers(&MatchFinder);
   auto Factory = newFrontendActionFactory(&MatchFinder);
@@ -459,7 +468,7 @@
   ASSERT_EQ(Changes.size(), 1u);
   const llvm::Any &Metadata = Changes[0].getMetadata();
   ASSERT_TRUE(llvm::any_isa(Metadata));
-  EXPECT_THAT(llvm::any_cast(Metadata), 17);
+  EXPECT_THAT(llvm::any_cast(Metadata), 5);
 }
 
 TEST_F(TransformerTest, MultiChange) {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -44,10 +44,13 @@
 auto Replacement = E.Replacement->eval(Result);
 if (!Replacement)
   return Replacement.takeError();
+auto Metadata = E.Metadata(Result);
+if (!Metadata)
+  return Metadata.takeError();
 transformer::Edit T;
 T.Range = *EditRange;
 T.Replacement = std::move(*Replacement);
-T.Metadata = E.Metadata;
+T.Metadata = std::move(*Metadata);
 Edits.push_back(std::move(T));
   }
   return Edits;
Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -47,6 +47,8 @@
 
 using TextGenerator = std::shared_ptr>;
 
+using AnyGenerator = MatchConsumer;
+
 // Description of a source-code edit, expressed in terms of an AST node.
 // Includes: an ID for the (bound) node, a selector for source related to the
 // node, a replacement and, optionally, an explanation for the edit.
@@ -87,7 +89,9 @@
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
-  llvm::Any Metadata;
+  AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &) {
+return llvm::Any();
+  };
 };
 
 /// Lifts a list of `ASTEdit`s into an `EditGenerator`.
@@ -261,9 +265,16 @@
 /// Removes the source selected by \p S.
 ASTEdit remove(RangeSelector S);
 
-inline ASTEdit withMetadata(ASTEdit edit, llvm::Any Metadata) {
-  edit.Metadata = std::move(Metadata);
-  return edit;
+// TODO(asoffer): If this returns an llvm::Expected, should we unwrap?
+template 
+inline ASTEdit withMetadata(ASTEdit Edit, Callable Metadata) {
+  Edit.Metadata =
+  [Gen = std::move(Metadata)](
+  const ast_matchers::MatchFinder::MatchResult &R) -> llvm::Any {
+return Gen(R);
+  };
+
+  return Edit;
 }
 
 /// The following three functions are a low-level part of the RewriteRule
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82226: Add Metadata to Transformer tooling

2020-06-29 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 274183.
asoffer added a comment.

Signed-comparison issue in test fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82226/new/

https://reviews.llvm.org/D82226

Files:
  clang/include/clang/Tooling/Refactoring/AtomicChange.h
  clang/include/clang/Tooling/Transformer/RewriteRule.h
  clang/lib/Tooling/Refactoring/AtomicChange.cpp
  clang/lib/Tooling/Transformer/RewriteRule.cpp
  clang/lib/Tooling/Transformer/Transformer.cpp
  clang/unittests/Tooling/RefactoringTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -439,6 +439,29 @@
   Input, Expected);
 }
 
+TEST_F(TransformerTest, WithMetadata) {
+  std::string Input = R"cc(
+int f() {
+  int x = 5;
+  return 7;
+}
+  )cc";
+
+  Transformer T(
+  makeRule(declStmt().bind("decl"),
+   withMetadata(remove(statement(std::string("decl"))), 17)),
+  consumer());
+  T.registerMatchers(&MatchFinder);
+  auto Factory = newFrontendActionFactory(&MatchFinder);
+  EXPECT_TRUE(runToolOnCodeWithArgs(
+  Factory->create(), Input, std::vector(), "input.cc",
+  "clang-tool", std::make_shared(), {}));
+  ASSERT_EQ(Changes.size(), 1u);
+  const llvm::Any &Metadata = Changes[0].getMetadata();
+  ASSERT_TRUE(llvm::any_isa(Metadata));
+  EXPECT_THAT(llvm::any_cast(Metadata), 17);
+}
+
 TEST_F(TransformerTest, MultiChange) {
   std::string Input = R"cc(
 void foo() {
Index: clang/unittests/Tooling/RefactoringTest.cpp
===
--- clang/unittests/Tooling/RefactoringTest.cpp
+++ clang/unittests/Tooling/RefactoringTest.cpp
@@ -1296,6 +1296,18 @@
   Replacement(Context.Sources, SourceLocation(), 0, "b")));
 }
 
+TEST_F(AtomicChangeTest, Metadata) {
+  AtomicChange Change(Context.Sources, DefaultLoc, 17);
+  const llvm::Any &Metadata = Change.getMetadata();
+  ASSERT_TRUE(llvm::any_isa(Metadata));
+  EXPECT_EQ(llvm::any_cast(Metadata), 17);
+}
+
+TEST_F(AtomicChangeTest, NoMetadata) {
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  EXPECT_FALSE(Change.getMetadata().hasValue());
+}
+
 class ApplyAtomicChangesTest : public ::testing::Test {
 protected:
   ApplyAtomicChangesTest() : FilePath("file.cc") {
Index: clang/lib/Tooling/Transformer/Transformer.cpp
===
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -53,7 +53,7 @@
 auto ID = Result.SourceManager->getFileID(T.Range.getBegin());
 auto Iter = ChangesByFileID
 .emplace(ID, AtomicChange(*Result.SourceManager,
-  T.Range.getBegin()))
+  T.Range.getBegin(), T.Metadata))
 .first;
 auto &AC = Iter->second;
 if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -47,6 +47,7 @@
 transformer::Edit T;
 T.Range = *EditRange;
 T.Replacement = std::move(*Replacement);
+T.Metadata = E.Metadata;
 Edits.push_back(std::move(T));
   }
   return Edits;
Index: clang/lib/Tooling/Refactoring/AtomicChange.cpp
===
--- clang/lib/Tooling/Refactoring/AtomicChange.cpp
+++ clang/lib/Tooling/Refactoring/AtomicChange.cpp
@@ -204,6 +204,12 @@
   Key = FilePath + ":" + std::to_string(FileIDAndOffset.second);
 }
 
+AtomicChange::AtomicChange(const SourceManager &SM, SourceLocation KeyPosition,
+   llvm::Any M)
+: AtomicChange(SM, KeyPosition) {
+  Metadata = std::move(M);
+}
+
 AtomicChange::AtomicChange(std::string Key, std::string FilePath,
std::string Error,
std::vector InsertedHeaders,
Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -35,6 +35,7 @@
 struct Edit {
   CharSourceRange Range;
   std::string Replacement;
+  llvm::Any Metadata;
 };
 
 /// Maps a match result to a list of concrete edits (with possible
@@ -85,6 +86,7 @@
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
+  llvm::Any Metadata;
 };
 
 /// Lifts a list of `ASTEdit`s into an `EditGenerator`.
@@ -258,6 +260,11 @@
 /// Removes the source selected by \p S.
 ASTEdit remove(RangeSe

[PATCH] D82226: Add Metadata to Transformer tooling

2020-06-29 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer added a comment.

I'm not against a map, but I don't see the need for it right now. We don't have 
multiple stages that write to the same AtomicChange in any tool I'm aware of. 
Given that these are immutable after construction, I think just the Any is 
simpler. So long as we're willing to break things later (Does LLVM have a 
policy on API compatibility?) I think it's better to start with the simpler 
thing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82226/new/

https://reviews.llvm.org/D82226



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

[PATCH] D82226: Add Metadata to Transformer tooling

2020-06-25 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 273522.
asoffer added a comment.

Last diff was a mistake.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82226/new/

https://reviews.llvm.org/D82226

Files:
  clang/include/clang/Tooling/Refactoring/AtomicChange.h
  clang/include/clang/Tooling/Transformer/RewriteRule.h
  clang/lib/Tooling/Refactoring/AtomicChange.cpp
  clang/lib/Tooling/Transformer/RewriteRule.cpp
  clang/lib/Tooling/Transformer/Transformer.cpp
  clang/unittests/Tooling/RefactoringTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -439,6 +439,29 @@
   Input, Expected);
 }
 
+TEST_F(TransformerTest, WithMetadata) {
+  std::string Input = R"cc(
+int f() {
+  int x = 5;
+  return 7;
+}
+  )cc";
+
+  Transformer T(
+  makeRule(declStmt().bind("decl"),
+   withMetadata(remove(statement(std::string("decl"))), 17)),
+  consumer());
+  T.registerMatchers(&MatchFinder);
+  auto Factory = newFrontendActionFactory(&MatchFinder);
+  EXPECT_TRUE(runToolOnCodeWithArgs(
+  Factory->create(), Input, std::vector(), "input.cc",
+  "clang-tool", std::make_shared(), {}));
+  ASSERT_EQ(Changes.size(), 1);
+  const llvm::Any &Metadata = Changes[0].getMetadata();
+  ASSERT_TRUE(llvm::any_isa(Metadata));
+  EXPECT_THAT(llvm::any_cast(Metadata), 17);
+}
+
 TEST_F(TransformerTest, MultiChange) {
   std::string Input = R"cc(
 void foo() {
Index: clang/unittests/Tooling/RefactoringTest.cpp
===
--- clang/unittests/Tooling/RefactoringTest.cpp
+++ clang/unittests/Tooling/RefactoringTest.cpp
@@ -1296,6 +1296,18 @@
   Replacement(Context.Sources, SourceLocation(), 0, "b")));
 }
 
+TEST_F(AtomicChangeTest, Metadata) {
+  AtomicChange Change(Context.Sources, DefaultLoc, 17);
+  const llvm::Any &Metadata = Change.getMetadata();
+  ASSERT_TRUE(llvm::any_isa(Metadata));
+  EXPECT_EQ(llvm::any_cast(Metadata), 17);
+}
+
+TEST_F(AtomicChangeTest, NoMetadata) {
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  EXPECT_FALSE(Change.getMetadata().hasValue());
+}
+
 class ApplyAtomicChangesTest : public ::testing::Test {
 protected:
   ApplyAtomicChangesTest() : FilePath("file.cc") {
Index: clang/lib/Tooling/Transformer/Transformer.cpp
===
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -53,7 +53,7 @@
 auto ID = Result.SourceManager->getFileID(T.Range.getBegin());
 auto Iter = ChangesByFileID
 .emplace(ID, AtomicChange(*Result.SourceManager,
-  T.Range.getBegin()))
+  T.Range.getBegin(), T.Metadata))
 .first;
 auto &AC = Iter->second;
 if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -47,6 +47,7 @@
 transformer::Edit T;
 T.Range = *EditRange;
 T.Replacement = std::move(*Replacement);
+T.Metadata = E.Metadata;
 Edits.push_back(std::move(T));
   }
   return Edits;
Index: clang/lib/Tooling/Refactoring/AtomicChange.cpp
===
--- clang/lib/Tooling/Refactoring/AtomicChange.cpp
+++ clang/lib/Tooling/Refactoring/AtomicChange.cpp
@@ -204,6 +204,12 @@
   Key = FilePath + ":" + std::to_string(FileIDAndOffset.second);
 }
 
+AtomicChange::AtomicChange(const SourceManager &SM, SourceLocation KeyPosition,
+   llvm::Any M)
+: AtomicChange(SM, KeyPosition) {
+  Metadata = std::move(M);
+}
+
 AtomicChange::AtomicChange(std::string Key, std::string FilePath,
std::string Error,
std::vector InsertedHeaders,
Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -35,6 +35,7 @@
 struct Edit {
   CharSourceRange Range;
   std::string Replacement;
+  llvm::Any Metadata;
 };
 
 /// Maps a match result to a list of concrete edits (with possible
@@ -85,6 +86,7 @@
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
+  llvm::Any Metadata;
 };
 
 /// Lifts a list of `ASTEdit`s into an `EditGenerator`.
@@ -258,6 +260,11 @@
 /// Removes the source selected by \p S.
 ASTEdit remove(RangeSelector S);
 
+i

[PATCH] D82226: Add Metadata to Transformer tooling

2020-06-25 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 273520.
asoffer marked an inline comment as done.
asoffer added a comment.

Updating other test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82226/new/

https://reviews.llvm.org/D82226

Files:
  clang/include/clang/Tooling/Refactoring/AtomicChange.h
  clang/include/clang/Tooling/Transformer/RewriteRule.h
  clang/lib/Tooling/Refactoring/AtomicChange.cpp
  clang/lib/Tooling/Transformer/RewriteRule.cpp
  clang/lib/Tooling/Transformer/Transformer.cpp
  clang/unittests/Tooling/RefactoringTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -439,6 +439,29 @@
   Input, Expected);
 }
 
+TEST_F(TransformerTest, WithMetadata) {
+  std::string Input = R"cc(
+int f() {
+  int x = 5;
+  return 7;
+}
+  )cc";
+
+  Transformer T(
+  makeRule(declStmt().bind("decl"),
+   withMetadata(remove(statement(std::string("decl"))), 17)),
+  consumer());
+  T.registerMatchers(&MatchFinder);
+  auto Factory = newFrontendActionFactory(&MatchFinder);
+  EXPECT_TRUE(runToolOnCodeWithArgs(
+  Factory->create(), Input, std::vector(), "input.cc",
+  "clang-tool", std::make_shared(), {}));
+  ASSERT_EQ(Changes.size(), 1);
+  const llvm::Any &Metadata = Changes[0].getMetadata();
+  ASSERT_TRUE(llvm::any_isa(Metadata));
+  EXPECT_THAT(llvm::any_cast(Metadata), 17);
+}
+
 TEST_F(TransformerTest, MultiChange) {
   std::string Input = R"cc(
 void foo() {
Index: clang/unittests/Tooling/RefactoringTest.cpp
===
--- clang/unittests/Tooling/RefactoringTest.cpp
+++ clang/unittests/Tooling/RefactoringTest.cpp
@@ -1296,6 +1296,22 @@
   Replacement(Context.Sources, SourceLocation(), 0, "b")));
 }
 
+struct SomeMetadata {
+  int Data;
+};
+
+TEST_F(AtomicChangeTest, Metadata) {
+  AtomicChange Change(Context.Sources, DefaultLoc, SomeMetadata{17});
+  const llvm::Any &Metadata = Change.getMetadata();
+  ASSERT_TRUE(llvm::any_isa(Metadata));
+  EXPECT_EQ(llvm::any_cast(Metadata).Data, 17);
+}
+
+TEST_F(AtomicChangeTest, NoMetadata) {
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  EXPECT_FALSE(Change.getMetadata().hasValue());
+}
+
 class ApplyAtomicChangesTest : public ::testing::Test {
 protected:
   ApplyAtomicChangesTest() : FilePath("file.cc") {
Index: clang/lib/Tooling/Transformer/Transformer.cpp
===
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -53,7 +53,7 @@
 auto ID = Result.SourceManager->getFileID(T.Range.getBegin());
 auto Iter = ChangesByFileID
 .emplace(ID, AtomicChange(*Result.SourceManager,
-  T.Range.getBegin()))
+  T.Range.getBegin(), T.Metadata))
 .first;
 auto &AC = Iter->second;
 if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -47,6 +47,7 @@
 transformer::Edit T;
 T.Range = *EditRange;
 T.Replacement = std::move(*Replacement);
+T.Metadata = E.Metadata;
 Edits.push_back(std::move(T));
   }
   return Edits;
Index: clang/lib/Tooling/Refactoring/AtomicChange.cpp
===
--- clang/lib/Tooling/Refactoring/AtomicChange.cpp
+++ clang/lib/Tooling/Refactoring/AtomicChange.cpp
@@ -204,6 +204,12 @@
   Key = FilePath + ":" + std::to_string(FileIDAndOffset.second);
 }
 
+AtomicChange::AtomicChange(const SourceManager &SM, SourceLocation KeyPosition,
+   llvm::Any M)
+: AtomicChange(SM, KeyPosition) {
+  Metadata = std::move(M);
+}
+
 AtomicChange::AtomicChange(std::string Key, std::string FilePath,
std::string Error,
std::vector InsertedHeaders,
Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -35,6 +35,7 @@
 struct Edit {
   CharSourceRange Range;
   std::string Replacement;
+  llvm::Any Metadata;
 };
 
 /// Maps a match result to a list of concrete edits (with possible
@@ -85,6 +86,7 @@
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
+  llvm::Any Metadata;
 };
 
 /// Lifts a list of `ASTEdit`s into an `EditGenerator`.
@@

[PATCH] D82226: Add Metadata to Transformer tooling

2020-06-25 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer updated this revision to Diff 273516.
asoffer marked an inline comment as done and an inline comment as not done.
asoffer added a comment.

Commenting fields and simplifying test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82226/new/

https://reviews.llvm.org/D82226

Files:
  clang/include/clang/Tooling/Refactoring/AtomicChange.h
  clang/include/clang/Tooling/Transformer/RewriteRule.h
  clang/lib/Tooling/Refactoring/AtomicChange.cpp
  clang/lib/Tooling/Transformer/RewriteRule.cpp
  clang/lib/Tooling/Transformer/Transformer.cpp
  clang/unittests/Tooling/RefactoringTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -439,6 +439,29 @@
   Input, Expected);
 }
 
+TEST_F(TransformerTest, WithMetadata) {
+  std::string Input = R"cc(
+int f() {
+  int x = 5;
+  return 7;
+}
+  )cc";
+
+  Transformer T(
+  makeRule(declStmt().bind("decl"),
+   withMetadata(remove(statement(std::string("decl"))), 17)),
+  consumer());
+  T.registerMatchers(&MatchFinder);
+  auto Factory = newFrontendActionFactory(&MatchFinder);
+  EXPECT_TRUE(runToolOnCodeWithArgs(
+  Factory->create(), Input, std::vector(), "input.cc",
+  "clang-tool", std::make_shared(), {}));
+  ASSERT_EQ(Changes.size(), 1);
+  const llvm::Any &Metadata = Changes[0].getMetadata();
+  ASSERT_TRUE(llvm::any_isa(Metadata));
+  EXPECT_THAT(llvm::any_cast(Metadata), 17);
+}
+
 TEST_F(TransformerTest, MultiChange) {
   std::string Input = R"cc(
 void foo() {
Index: clang/unittests/Tooling/RefactoringTest.cpp
===
--- clang/unittests/Tooling/RefactoringTest.cpp
+++ clang/unittests/Tooling/RefactoringTest.cpp
@@ -1296,6 +1296,22 @@
   Replacement(Context.Sources, SourceLocation(), 0, "b")));
 }
 
+struct SomeMetadata {
+  int Data;
+};
+
+TEST_F(AtomicChangeTest, Metadata) {
+  AtomicChange Change(Context.Sources, DefaultLoc, SomeMetadata{17});
+  const llvm::Any &Metadata = Change.getMetadata();
+  ASSERT_TRUE(llvm::any_isa(Metadata));
+  EXPECT_EQ(llvm::any_cast(Metadata).Data, 17);
+}
+
+TEST_F(AtomicChangeTest, NoMetadata) {
+  AtomicChange Change(Context.Sources, DefaultLoc);
+  EXPECT_FALSE(Change.getMetadata().hasValue());
+}
+
 class ApplyAtomicChangesTest : public ::testing::Test {
 protected:
   ApplyAtomicChangesTest() : FilePath("file.cc") {
Index: clang/lib/Tooling/Transformer/Transformer.cpp
===
--- clang/lib/Tooling/Transformer/Transformer.cpp
+++ clang/lib/Tooling/Transformer/Transformer.cpp
@@ -53,7 +53,7 @@
 auto ID = Result.SourceManager->getFileID(T.Range.getBegin());
 auto Iter = ChangesByFileID
 .emplace(ID, AtomicChange(*Result.SourceManager,
-  T.Range.getBegin()))
+  T.Range.getBegin(), T.Metadata))
 .first;
 auto &AC = Iter->second;
 if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
Index: clang/lib/Tooling/Transformer/RewriteRule.cpp
===
--- clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -47,6 +47,7 @@
 transformer::Edit T;
 T.Range = *EditRange;
 T.Replacement = std::move(*Replacement);
+T.Metadata = E.Metadata;
 Edits.push_back(std::move(T));
   }
   return Edits;
Index: clang/lib/Tooling/Refactoring/AtomicChange.cpp
===
--- clang/lib/Tooling/Refactoring/AtomicChange.cpp
+++ clang/lib/Tooling/Refactoring/AtomicChange.cpp
@@ -204,6 +204,12 @@
   Key = FilePath + ":" + std::to_string(FileIDAndOffset.second);
 }
 
+AtomicChange::AtomicChange(const SourceManager &SM, SourceLocation KeyPosition,
+   llvm::Any M)
+: AtomicChange(SM, KeyPosition) {
+  Metadata = std::move(M);
+}
+
 AtomicChange::AtomicChange(std::string Key, std::string FilePath,
std::string Error,
std::vector InsertedHeaders,
Index: clang/include/clang/Tooling/Transformer/RewriteRule.h
===
--- clang/include/clang/Tooling/Transformer/RewriteRule.h
+++ clang/include/clang/Tooling/Transformer/RewriteRule.h
@@ -35,6 +35,7 @@
 struct Edit {
   CharSourceRange Range;
   std::string Replacement;
+  llvm::Any Metadata;
 };
 
 /// Maps a match result to a list of concrete edits (with possible
@@ -85,6 +86,7 @@
   RangeSelector TargetRange;
   TextGenerator Replacement;
   TextGenerator Note;
+  llvm::Any Metadata;
 };
 
 /// L

[PATCH] D82226: Add Metadata to Transformer tooling

2020-06-25 Thread Andy Soffer via Phabricator via cfe-commits 
asoffer marked 2 inline comments as done.
asoffer added a comment.

In D82226#2114111 , @ymandel wrote:

> Looks good! Only real question is one of design -- should we consider the 
> (deeper) change of templating the various types rather than using dynamic 
> typing?  For that matter, the decision doesn't even have to be the same for 
> both AtomicChange and the Transformer types. Transformer could be templated 
> while AtomicChange uses any.


I think the tradeoff here is 
Dynamic typing -- faster compile times, type safety checked at run-time (in 
tests), lower maintenance cost
Templates -- Faster runtime, type safety checked at compile-time, better user 
expereience

I don't think the runtime speed is particularly important here in the sense 
that it's unlikely to be the bottleneck. Given the crash-on-invalid-type 
semantics, we're essentially trading off usability for maintenance cost. In 
this case I don't think asking users to do the extra cast is a significant 
enough burden to warrant the cost of templating everything.

I also don't strongly object to it. The nice thing about llvm::Any, is that 
this can be mostly changed incrementally if we later decide that's appropriate 
(only having to change the one getMetadata call at the call-site).




Comment at: clang/lib/Tooling/Refactoring/AtomicChange.cpp:210
+: AtomicChange(SM, KeyPosition) {
+  Metadata = std::move(M);
+}

ymandel wrote:
> I assume that  
> ```
> : AtomicChange(SM, KeyPosition), Metadata(std::move(M)) {}
> ```
> didn't work?
Correct. You can't use a forwarding constructor followed by a field initializer 
(which I learned by attempt it on this PR).



Comment at: clang/unittests/Tooling/TransformerTest.cpp:460
+  auto Factory = newFrontendActionFactory(&MatchFinder);
+  EXPECT_TRUE(runToolOnCodeWithArgs(
+  Factory->create(), Input, std::vector(), "input.cc",

ymandel wrote:
> Why expect vs assert?
EXPECT when the test can reasonably continue to produce useful output without 
crashing. So here, if the tool fails, we can still safely access Changes, but 
if Changes.size() != 1, we can't safely index into it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82226/new/

https://reviews.llvm.org/D82226



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