klimek added inline comments.
Comment at:
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h:23-37
+/// A common refactoring action rule interface.
+class RefactoringActionRule {
+public:
+ enum RuleKind { SourceChangeRefactoringRuleKind };
+
+ RuleKind
arphaman added inline comments.
Comment at:
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h:23-37
+/// A common refactoring action rule interface.
+class RefactoringActionRule {
+public:
+ enum RuleKind { SourceChangeRefactoringRuleKind };
+
+ RuleKind
klimek added inline comments.
Comment at:
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h:23-37
+/// A common refactoring action rule interface.
+class RefactoringActionRule {
+public:
+ enum RuleKind { SourceChangeRefactoringRuleKind };
+
+ RuleKind
This revision was automatically updated to reflect the committed changes.
Closed by commit rL311884: [refactor] initial support for refactoring action
rules (authored by arphaman).
Changed prior to commit:
https://reviews.llvm.org/D36075?vs=112678=112879#toc
Repository:
rL LLVM
arphaman added a comment.
Thanks,
I'll start working on the documentation patch and will split follow-up
`clang-refactor` patch into one or two parts today.
Comment at:
include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:33
+public:
+ virtual
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
Thanks for the changes! Lgtm with a few nits.
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:33
+///
+/// - requiredSelection: The refactoring
arphaman updated this revision to Diff 112678.
arphaman added a comment.
- Get rid of `RefactoringResult` in favour of different rule types.
- Rename `RefactoringOperation` to `RefactoringRuleContext`.
- Address a couple more comments.
Repository:
rL LLVM
https://reviews.llvm.org/D36075
arphaman marked 5 inline comments as done.
arphaman added inline comments.
Comment at: include/clang/Tooling/Refactoring/RefactoringActionRules.h:33
+///
+/// - requiredSelection: The refactoring function won't be invoked unless the
+/// given selection
alexshap added a comment.
@arphaman
The selection requirement is supposed to be orthogonal to AST matchers, not a
replacement. It should be used when working with source selection in editors.
I did mess around with moving over clang-reorder-fields using this kind of
model and didn't see
arphaman added inline comments.
Comment at: lib/Tooling/Refactoring/SourceSelectionConstraints.cpp:13
+
+using namespace clang;
+using namespace tooling;
ioeric wrote:
> We are tempted to avoid `using namespace` if possible.
Why? It's not in a header. `using
arphaman added inline comments.
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:21
+struct RefactoringResult {
+ enum ResultKind {
+/// A set of source replacements represented using a vector of
ioeric wrote:
> arphaman wrote:
> > ioeric
ioeric added inline comments.
Comment at:
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:26
+template
+detail::SourceSelectionRequirement<
+typename selection::detail::EvaluateSelectionChecker<
arphaman wrote:
> ioeric wrote:
> >
arphaman updated this revision to Diff 112566.
arphaman marked 2 inline comments as done.
arphaman added a comment.
- Rename `detail` to `internal`.
- Remove the `BaseSpecializedRule`.
- Simplify selection requirement and constraint evaluation.
Repository:
rL LLVM
arphaman added inline comments.
Comment at:
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:26
+template
+detail::SourceSelectionRequirement<
+typename selection::detail::EvaluateSelectionChecker<
ioeric wrote:
> Could you help me
arphaman added a comment.
In https://reviews.llvm.org/D36075#851278, @ioeric wrote:
> Thanks for the changes! The code is much clearer.
>
> I am wondering if the current design could be extended to support tools (or
> rules) that use AST matchers? Or is the selection expected to be powerful
>
arphaman added inline comments.
Comment at: include/clang/Tooling/Refactoring/RefactoringResult.h:21
+struct RefactoringResult {
+ enum ResultKind {
+/// A set of source replacements represented using a vector of
ioeric wrote:
> I'm a bit unsure about the
ioeric added a comment.
Thanks for the changes! The code is much clearer.
I am wondering if the current design could be extended to support tools (or
rules) that use AST matchers? Or is the selection expected to be powerful
enough to replace AST matchers?
We have a few tools in
arphaman updated this revision to Diff 112195.
arphaman marked 7 inline comments as done.
arphaman added a comment.
- Split the header
- Remove DiagnosticOr in favour of Expected that will use a DiagnosticError
that was proposed in the other patch.
- Address the other review comments
arphaman added inline comments.
Comment at:
include/clang/Tooling/Refactoring/RefactoringOperationController.h:19
+
+/// Encapsulates all of the possible state that an individual refactoring
+/// operation might have. Controls the process of initiation of refactoring
arphaman added a comment.
Extracted `DiagnosticOr` to a separate patch at https://reviews.llvm.org/D36969.
I will update this patch tomorrow.
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
alexshap added inline comments.
Comment at: include/clang/Basic/DiagnosticOr.h:40
+ /// diagnostic.
+ DiagnosticOr(PartialDiagnosticAt Diagnostic) : HasDiagnostic(true) {
+new (getDiagnosticStorage()) PartialDiagnosticAt(std::move(Diagnostic));
explicit ?
ioeric added a comment.
This is great work and definitely a lot to digest! ;) Some high-level comments
for the first round.
In general, I really appreciate the high-level interfaces; I am just a bit
concerned about the implementation which is a bit hard to follow at this point,
especially
arphaman added a comment.
Ping.
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
arphaman updated this revision to Diff 110563.
arphaman edited the summary of this revision.
arphaman added a comment.
- Simplify error/diagnostic handling. Use `DiagnosticOr` instead of
`DiagOr`.
- Simplify the code for the selection requirements by removing lambda deducers
and instead using
arphaman added a comment.
+ Introduce refactoring diagnostics.
Repository:
rL LLVM
https://reviews.llvm.org/D36075
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
arphaman created this revision.
Herald added a subscriber: mgorny.
This patch implements a couple of functions that were described in my RFC from
last week that’s available at
http://lists.llvm.org/pipermail/cfe-dev/2017-July/054831.html. This patch adds
the following pieces: `apply` function,
26 matches
Mail list logo
