[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe5b3202b6f94: [libTooling] In Clang Transformer, change
`Metadata` field to deferred… (authored by asoffer, committed by ymandel).
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 ) -> 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();
auto Factory = newFrontendActionFactory();
@@ -459,7 +468,7 @@
ASSERT_EQ(Changes.size(), 1u);
const llvm::Any = 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 ) -> 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
[PATCH] D83820: Change metadata to deferred evalutaion in Clang Transformer.
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 ) -> 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();
auto Factory = newFrontendActionFactory();
@@ -459,7 +468,7 @@
ASSERT_EQ(Changes.size(), 1u);
const llvm::Any = 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 ) -> 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.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc0b8954ecba5: [libTooling] In Clang Transformer, change
`Metadata` field to deferred… (authored by ymandel).
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 ) -> 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();
auto Factory = newFrontendActionFactory();
@@ -459,7 +468,7 @@
ASSERT_EQ(Changes.size(), 1u);
const llvm::Any = 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 ) -> 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.
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 ) -> 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();
auto Factory = newFrontendActionFactory();
@@ -459,7 +468,7 @@
ASSERT_EQ(Changes.size(), 1u);
const llvm::Any = 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 ) -> 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.
ymandel 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
+ // should not be requierd to do so.
+ AnyGenerator Metadata = [](const ast_matchers::MatchFinder::MatchResult &) {
typo: requierd
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.
ymandel 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 &) {
asoffer wrote:
> 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.
Sure. The lack of null check is consistent with the other two fields. so, that
just leaves it inconsistent with `Note`, which doesn't have as easy an answer
(since string isn't nullable). Given that `Note` isn't used (yet), let's punt
on the consistency issue until we deal with `Note` properly. You're good to go.
Thanks for talking this through!
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.
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.
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 ) -> 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();
auto Factory = newFrontendActionFactory();
@@ -459,7 +468,7 @@
ASSERT_EQ(Changes.size(), 1u);
const llvm::Any = 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 ) -> 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.
ymandel marked an inline comment as done.
ymandel 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 &) {
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();
}
```
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
asoffer wrote:
> 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.
Punting is fine with me. I'm not against making the user explicitly create the
`Any`, but I don't have a strong opinion on this.
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.
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.
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 ) -> 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();
auto Factory = newFrontendActionFactory();
@@ -459,7 +468,7 @@
ASSERT_EQ(Changes.size(), 1u);
const llvm::Any = 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 ) -> 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.
ymandel added a comment.
Looks like your changes to the .cpp and test files were reverted...
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 &) {
nit: typos
The same applies to `Note`. Since this is a nullable type, can we ask that the
user check `Metadata != nullptr`?
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
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`.
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.
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.
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.
ymandel accepted this revision.
ymandel 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();
Why default, especially when we don't default the others? Is it worth a comment?
Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:269
+// TODO(asoffer): If this returns an llvm::Expected, should we unwrap?
+template
+inline ASTEdit withMetadata(ASTEdit Edit, Callable Metadata) {
Why generalize to support any callable? Perhaps worth explaining in comment?
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.
gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.
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
What is "this" who is "we"? Should this be an implementation comment instead
(in the function body next to the relevant line)?
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.
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 ) -> 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();
auto Factory = newFrontendActionFactory();
@@ -459,7 +468,7 @@
ASSERT_EQ(Changes.size(), 1u);
const llvm::Any = 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 ) -> 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
