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<T>` the `AnyGenerator` will
+// construct an `llvm::Expected<llvm::Any>` 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<R(MatchResult)>`, 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 <typename Callable>
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<T>` the `AnyGenerator` will
+// construct an `llvm::Expected<llvm::Any>` 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<R(MatchResult)>`, 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 <typename Callable>
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
