This is an archive of the discontinued LLVM Phabricator instance.

[LibTooling] Add Transformer, a library for source-to-source transformations.
ClosedPublic

Authored by ymandel on Mar 14 2019, 11:20 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ymandel created this revision.Mar 14 2019, 11:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 11:20 AM

Looks very polished, thanks!
Will have to sink the change in a bit, will come back with more comments on Monday.
In the meantime, a few initial comments and suggestions.

clang/include/clang/Tooling/Refactoring/Transformer.h
54 ↗(On Diff #190688)

Intuitively, it feels that any filtering should be possible at the level of the AST matchers. Is that not the case?
Could you provide some motivating examples where AST matchers cannot be used to nail down the matching nodes and we need MatchFilter?

Please note I have limited experience with AST matchers, so there might be some obvious things that I'm missing.

135 ↗(On Diff #190688)

Maybe consider separating the fluent API to build the rewrite rule from the rewrite rule itself?

Not opposed to having the fluent builder API, this does look nice and seems to nicely align with the matcher APIs.
However, it is somewhat hard to figure out what can RewriteRule do after it was built when looking at the code right now.

class RewriteRule {
public:
  RewriteRule(DynTypedMatcher, TextGenerator Replacement, TextGenerator Explanation);

  DynTypedMatcher matcher() const;
  Expected<string> replacement() const;
  Expected<string> explanation() const;
};

struct RewriteRuleBuilder { // Having a better name than 'Builder' would be nice.
  RewriteRule finish() &&; // produce the final RewriteRule.

  template <typename T>
  RewriteRuleBuilder &change(const TypedNodeId<T> &Target,
                      NodePart Part = NodePart::Node) &;
  RewriteRuleBuilder &replaceWith(TextGenerator Replacement) &;
  RewriteRuleBuilder &because(TextGenerator Explanation) &;
private:
  RewriteRule RuleInProgress;
};
RewriteRuleBuilder makeRewriteRule();
ymandel marked 4 inline comments as done.Mar 19 2019, 11:00 AM
ymandel added inline comments.
clang/include/clang/Tooling/Refactoring/Transformer.h
54 ↗(On Diff #190688)

Good point. The examples I have would actually be perfectly suited to a matcher. That said, there is not matcher support for a simple predicate of this form, along the lines of gtest's Truly(predicate). I'll remove this and separately try to add something like Truly to the matchers.

135 ↗(On Diff #190688)

I see your point, but do you think it might be enough to improve the comments on the class? My concern with a builder is the mental burden on the user of another concept (the builder) and the need for an extra .build() everywhere. To a lesser extent, I also don't love the cost of an extra copy, although I doubt it matters and I suppose we could support moves in the build method.

ilya-biryukov added inline comments.Mar 20 2019, 10:21 AM
clang/include/clang/Tooling/Refactoring/Transformer.h
135 ↗(On Diff #190688)

The issues with the builder interface is that it requires lots of boilerplate, which is hard to throw away when reading the code of the class. I agree that having a separate builder class is also annoying (more concepts, etc).

Keeping them separate would be my personal preference, but if you'd prefer to keep it in the same class than maybe move the non-builder pieces to the top of the class and separate the builder methods with a comment.
WDYT?

To a lesser extent, I also don't love the cost of an extra copy, although I doubt it matters and I suppose we could support moves in the build method.

I believe we can be as efficient (up to an extra move) with builders as without them. If most usages are of the form RewriteRule R = rewriteRule(...).change(...).replaceWith(...).because(...);
Then we could make all builder functions return r-value reference to a builder and have an implicit conversion operator that would consume the builder, producing the final RewriteRule:

class RewriteRuleBuilder {
  operator RewriteRule () &&;
  /// ...
};
RewriteRuleBuilder rewriteRule();

void addRule(RewriteRule R);
void clientCode() {
  addRule(rewriteRule().change(Matcher).replaceWith("foo").because("bar"));
}
ymandel marked 4 inline comments as done.Mar 20 2019, 10:57 AM
ymandel added inline comments.
clang/include/clang/Tooling/Refactoring/Transformer.h
135 ↗(On Diff #190688)

I didn't realize that implicit conversion functions are allowed. With that, I'm fine w/ splitting. Thanks!

I've just realized the review is probably missing the latest revision.
@ymandel, could you upload the new version?

clang/include/clang/Tooling/Refactoring/Transformer.h
54 ↗(On Diff #190688)

Makes sense! Maybe put a FIXME here to let the readers know this is moving to the ast matchers?

57 ↗(On Diff #190688)

Note: I'm probably not seeing the latest version, so this comment might be outdated. Feel free to ignore if you've already moved this to the matchers.

BTW, I was wondering whether MatchFilter carries its weight? What are the pros over:

using MatchFilter = std::function<bool(const ast_matchers::MatchFinder::MatchResult &Result)>;

The only thing thats come to mind is that default-constructed function cannot be called, which is less useful than returning true in a default-constructed.
We could handle this in the builder once, the rewrite rule can assert the predicate is always set.

Do you have other reasons to have it in mind that I'm missing?

85 ↗(On Diff #190688)

What happens if member is composed of multiple tokens? E.g.

foo.bar::baz; // member tokens are ['bar', '::', 'baz']
foo.template baz<int>; // member tokens are ['template', 'baz', '<', 'int', '>']
foo.operator +; // member tokens are ['operator', '+']

I might be misinterpreting the meaning of "member" token.

89 ↗(On Diff #190688)

Same here, what happens to the template arguments and multi-token names, e.g.
operator + or foo<int, int>?

135 ↗(On Diff #190688)

Have you uploaded the new version? I don't seem to see the split.

ilya-biryukov added inline comments.Mar 22 2019, 10:18 AM
clang/unittests/Tooling/TransformerTest.cpp
157 ↗(On Diff #190688)

NIT: maybe consider adding overloads for these function that allow to pass char literals there? This is probably a common case, so it'd be nice to write this as:

replaceWith("REPLACED").because("Use size() method directly on string.");
ymandel marked an inline comment as done.Mar 22 2019, 12:44 PM

Working on the new version now. Will note with "PTAL" once that's ready. Sorry that wasn't clear in earlier responses.

Working on the new version now. Will note with "PTAL" once that's ready.

Got you! Waiting for the new revision.

Sorry that wasn't clear in earlier responses.

Or maybe it's me misreading earlier responses. Happy to review the new changes as soon as they land.

ymandel updated this revision to Diff 192172.Mar 25 2019, 11:40 AM
ymandel marked an inline comment as done.

Split RewriteRule into two classes, remove support for where clause, support multi-token targets, and add corresponding tests.

ymandel marked 10 inline comments as done.Mar 25 2019, 11:47 AM

Addressed the most major comments. Still working on some smaller ones. PTAL. Thanks!

clang/include/clang/Tooling/Refactoring/Transformer.h
54 ↗(On Diff #190688)

I've removed the where clause entirely. I'll separately add the matcher support (I've figured out how to do it within the existing framework).

85 ↗(On Diff #190688)

Good catch! I've updated to handle these correctly (I believe). Added some tests, plan to add some more.

89 ↗(On Diff #190688)

Good point. This seems difficult to get right, since NamedDecl does not carry sufficient loc data. However, I've updated the code to explicitly fail in such cases, so at least we won't have bad rewrites.

BTW, any idea whether constructor initializers can ever be multi token?

135 ↗(On Diff #190688)

I have now. FWIW, I've left both ref overloads, but what do you think of dropping the lvalue-ref overloads and only supporting rvalue refs? Users can still pretty easily use an lvalue, just by inserting a trivial std::move() around the lvalue.

clang/unittests/Tooling/TransformerTest.cpp
157 ↗(On Diff #190688)

Sure, will do in next revision. The version that's integrated w/ stencils does exactly this, btw. I wonder, though, whether we should add an implicit constructor to the TextGenerator class instead of adding overloads. I have a general distrust of implicit constructors, but I see that's not the case in the clang codebase. WDYT?

ilya-biryukov added inline comments.Mar 26 2019, 2:32 AM
clang/include/clang/Tooling/Refactoring/Transformer.h
89 ↗(On Diff #190688)

BTW, any idea whether constructor initializers can ever be multi token?

Two cases come to mind:

  1. arbitrary names when initializing base classes, something like ::ns::X<int>(10)
  2. template packs with ellipsis (although ellipsis shouldn't be normally part that we replace, I guess): Base(10)...

Full example:

namespace ns {
  struct X {
    X(int);
  };
}


template <class ...Bases>
struct Y : ns::X, Bases... {
  Y() : ns::X(10), Bases(10)... {
  }
};

struct Z {
  Z(int);
};
struct W {
  W(int);
};

Y<Z, W> y;
ymandel updated this revision to Diff 192267.Mar 26 2019, 7:00 AM
ymandel marked 5 inline comments as done.
  • Remove lvalue-ref overloads in builder
  • add StringRef overloads for TextGenerator-taking methods
  • constrain Name targeting for ctor initializers to explicitly written initializers.
  • add multi-token member test
ymandel marked an inline comment as done.Mar 26 2019, 7:04 AM
ymandel added inline comments.
clang/include/clang/Tooling/Refactoring/Transformer.h
89 ↗(On Diff #190688)

Turns out the code was already filtering these cases. I added an addition constrain of I->isWritten() for initializers. So, only explicit initialization of fields is allowed, and therefore I'd venture guaranteed to be a single token. I noticed that Kythe seems to make the same assumption.

That said, I could change to code to specify the range as a char-range of getMemberLoc(), getLParenLoc() if we can't rely on that (single-token) guarantee.

ilya-biryukov marked an inline comment as done.Mar 27 2019, 12:19 PM

Mostly nits and small API questions form my side.
This looks very good overall! I'm planning to take a closer look at the handling of macros and various AST nodes in more detail this week, but the approach looks solid from a higher level.

clang/include/clang/Tooling/Refactoring/Transformer.h
38 ↗(On Diff #192267)

I'm not sure if this is in the LLVM style guide, but we might want to avoid introducing these names into clang::tooling namespaces in the headers.

My fear is that users will rely on those using without knowing that explicitly and won't add corresponding using directives or qualifiers to their .cpp files, making refactoring and moving the code around harder.

Could you fully-qualify those names in the header instead? There does not seem to be too many of them.

65 ↗(On Diff #192267)

Why would a TextGenerator fail?
I imagine all of the failure cases are programming errors (matchers in the rewrite rule were not aligned with the corresponding text generating function). For those cases, using the assert macro seems cleaner.

116 ↗(On Diff #192267)

NIT: maybe move all inits to the constructor?
To have all initializers in one place.

170 ↗(On Diff #192267)

NIT: maybe remove the =default copy and move ctors and assignments?
They should be generated automatically anyway, right?

191 ↗(On Diff #192267)

NIT: maybe accept a std::string to let the clients pass a std::string if they already have it, avoiding extra copies in some cases?

135 ↗(On Diff #190688)

Yeah, LG, having only a single overload means less boilerplate and the fluent-builder APIs are mostly used exclusively as r-values anyway.

clang/lib/Tooling/Refactoring/Transformer.cpp
28 ↗(On Diff #192267)

Other files in the tooling library seem to be adding using namespace clang instead of putting the declaration into a namespace.
Could you please change the new code to do the same for consistency?

30 ↗(On Diff #192267)

Why put using directives into an anonymous namespace?
I have not seen this pattern before, could you point me to explanations on why this is useful?

44 ↗(On Diff #192267)

NIT: use UpperCamelCase for local variable names to conform to LLVM style.

110 ↗(On Diff #192267)

NIT: maybe add the corresponding assertion at the start of the function?

110 ↗(On Diff #192267)

s/node/Node
s/target_part/TargetPart

111 ↗(On Diff #192267)

NIT: consider merging verifyTarget into getTarget and making getTarget return Expected<CharSourceRange>.
Would allow avoiding to write one of the complicated switches and error-checking arguably looks just as natural in the getTarget as it is in verifyTarget.

Also, having the invariants closer to the code using them makes it easier to certify both are correct, e.g. seeing that NamedDecl.isIdentifier() was checked before accessing the NamedDecl.getName() in the same function is simpler.

134 ↗(On Diff #192267)

NIT: start a fixme at a new line to make it more visible.
Or is it clang-format merging it into the previous line?

144 ↗(On Diff #192267)

This could be operator+ too, right?

struct X {
  X operator+(int);

  void test() {
    X (X::*ptr)(int) = &X::operator+;
  }
};

Would probably want to bail out in this case as well.

160 ↗(On Diff #192267)

The comment still mentions the where clause. A leftover from the previous version?

199 ↗(On Diff #192267)

Maybe use std::string in the public interface anyway?

  • This function is definitely not a bottleneck, so an extra copy is not a big deal for performance.
  • Using std::string would let the clients save a copy in their client code if they can (by using std::move or creating an r-value string in the first place).
  • We will have a copy in the body of our function (we have it anyway). We'll eliminate it after switching to C++14 in the body of our function without changing the clients.
228 ↗(On Diff #192267)

NIT: remove braces around a single statement.

ymandel updated this revision to Diff 192619.Mar 28 2019, 5:51 AM
ymandel marked 2 inline comments as done.

Simplified TextGenerator, changed namespace handling, collapsed verifyTarget() into getTarget(), and other changes in response to comments.

ymandel updated this revision to Diff 192623.Mar 28 2019, 6:14 AM
ymandel marked 26 inline comments as done.

Adjusted test to cover case discussed in comments.

Thanks for the detailed review and really helpful comments!

clang/include/clang/Tooling/Refactoring/Transformer.h
38 ↗(On Diff #192267)

right. I'd intended to introduce these into the clang tooling namespace -- that is, these weren't just convenience aliases for the header file. But, I no longer think that's useful in any case, so dropping them is certainly best.

65 ↗(On Diff #192267)

Sure. I could go either way. I think some of these cases fall on the border between an invariant violation and "invalid argument" or some such. But, let's keep it simpler for now.

116 ↗(On Diff #192267)

nicer, thanks.

170 ↗(On Diff #192267)

Sure. I was going based on google's style recommendations, but personally i prefer leaving them implicit.

clang/lib/Tooling/Refactoring/Transformer.cpp
28 ↗(On Diff #192267)

Done. Agreed about being consistent. FWIW, I can't say I like this style. Perhaps because I'm not used to it, but it feels too implicit. It forces the reader to figure out where each definition is being associated. Also, I discovered it only works for method definitions. Free functions still need to be explicitly namespaced.

Any idea what the reason for this style is?

30 ↗(On Diff #192267)

You gain an extra little bit of robustness against clashing with something declared in the enclosing namespace. But, this is overkill even for me -- I generally only do this when I already have an anonymous namespace; there's no good reason to create one for this purpose.

111 ↗(On Diff #192267)

Yes, I like it much better this way. The split wasn't worth it.

134 ↗(On Diff #192267)

Done.
It's emacs (c-fill-paragraph), clang-format is better about this, because it's very conservative about touching comments.

144 ↗(On Diff #192267)

The check E->getNameInfo().getName().isIdentifier() rules that out. It was hidden in verifyTarget(). I hope the new code makes this clearer?
Also, I updated the test TransformerTest.NodePartNameDeclRefFailure to cover this case.

160 ↗(On Diff #192267)

right. deleted the comment altogether.

199 ↗(On Diff #192267)

Agreed. Updated all relevant places.

ilya-biryukov marked 2 inline comments as done.Mar 29 2019, 5:59 AM

The only real question I have is about returning an error vs an empty transformation in of macros.
The rest are just NITs.

Thanks for the change! I'll get to the NodeId patch right away :-)

clang/include/clang/Tooling/Refactoring/Transformer.h
70 ↗(On Diff #192623)

NIT: maybe mention what it should be used for? we plan to show to to the user (e.g. in the clang-tidy fix description), right?

89 ↗(On Diff #192623)

Could you also add examples on how to apply the rewrite rule here?
So that the users can have an idea about the full workflow when reading the code.

97 ↗(On Diff #192623)

NIT: maybe assert this invariant in the constructor?

122 ↗(On Diff #192623)

This method does not seem to be used anywhere.
Are we missing the usages in this patch? Maybe remove it from the initial implementation and re-add later when we have the callsites?

175 ↗(On Diff #192623)

NIT: the top-level std::move looks redundant, maybe remove it?

170 ↗(On Diff #192267)

+1. Means less boilterplate.

clang/lib/Tooling/Refactoring/Transformer.cpp
31 ↗(On Diff #192623)

NIT: we could leave out the trailing :: for clang and llvm, they are well-known and unambiguous namespace names.

42 ↗(On Diff #192623)

NIT: could you add a brief comment on what this function is expected to return?

133 ↗(On Diff #192623)

NIT: remove namespaces for consistency with the rest of the code.

(Probably a leftover from the previous version)

150 ↗(On Diff #192623)

NIT: consider simply propagating the original error up the stack in that case to avoid boilerplate.
Although adding the .target() information to the error might be useful, so up to you.

157 ↗(On Diff #192623)

Why not return an error in case of macros too? Is there any use of the empty transformation to the clients?
Alternatively, we might want to document this behavior (applyRewriteRule can return empty transformations) and it's rationale.

28 ↗(On Diff #192267)

I personally have a slight preference towards using namespace style myself because it produces compiler errors whenever declaration and definition signatures don't match, catching errors early in cases where one forgets to update either of those, e.g.

// foo.h
namespace clang{
std::unique_ptr<Something> createSomething();
}

// foo1.cpp
using namespace clang;

std::unique_ptr<Something> clang::createSomething(int Param) { // <-- error
}


// foo2.cpp
namespace clang {
std::unique_ptr<Something> clang::createSomething(int Param) { // <-- all ok, will get linker errors on usages instead.
}
}

There might be other reasons and I'm not sure why exactly the code in clang uses this approach.

144 ↗(On Diff #192267)

Right, thanks! Moving the check into the code makes it more explicit now, thanks!

ilya-biryukov added inline comments.Mar 29 2019, 5:59 AM
clang/unittests/Tooling/TransformerTest.cpp
61 ↗(On Diff #192623)

Maybe put the whole file into an anonymous namespace?
To avoid potential name clashes with other test files (and ODR violations that might follow)

88 ↗(On Diff #192623)

NIT: remove redundant braces

159 ↗(On Diff #192623)

These three lines are in each test. Consider moving them to a helper function (e.g. returning the final result as text?) to reduce the boilerplate in the test.

ymandel updated this revision to Diff 192889.Mar 29 2019, 12:24 PM
ymandel marked an inline comment as done.
  • Assorted changes in response to comments.
ymandel marked 19 inline comments as done.Mar 29 2019, 12:33 PM

Thanks for (more) helpful comments. I think the code is a lot better now than it started out. :)

Also, converted RewriteRule to a simple struct as per our discussion.

clang/include/clang/Tooling/Refactoring/Transformer.h
70 ↗(On Diff #192623)

Done. But, given that we don't use this field yet, I'm fine deleting it until a future revision. I'm also fine leaving it given that we know we'll be needing it later.

89 ↗(On Diff #192623)

Is this what you had in mind? Unlike clang-tidy, there is no neat infrastructure into which we can drop it.

97 ↗(On Diff #192623)

The constructor sets this field, so no need to assert.

122 ↗(On Diff #192623)

It was used above in makeMatcher. But, it was overkill -- users can just reference RootId directly, so I've removed it.

175 ↗(On Diff #192623)

Yes, not sure why did that. thanks.

clang/lib/Tooling/Refactoring/Transformer.cpp
133 ↗(On Diff #192623)

This is necessary as far as I can tell. W/o it, I get a linker failure (missing definition). Based on the declaration in the header, the compiler resolves the reference below to clang::tooling::applyRewriteRule() but this definition ends up in the global namespace.

I think the using decls only work for method definitions -- the type seems to resolve to be the one in the namespace. But free functions don't get the same treatment. Unless I"m doing something wrong?

150 ↗(On Diff #192623)

integrated id into getTarget so we can avoid this error handling step.

157 ↗(On Diff #192623)

I think the common case is that we simply want to skip macros -- there's nothing erroneous about them, they're just usually hard to deal w/ correctly. That said, we might want to change this to *never* return an error and just assert when things go wrong. The advantage of the current design is that callers can in principle ignore failed rewrites.

However, in practice, if the rewrite fails that means it had a bug, so it might be best to asssert(). Only real advantage, then, is that this is easier to test since it doesn't crash the program.

WDYT?

clang/unittests/Tooling/TransformerTest.cpp
61 ↗(On Diff #192623)

also removed static qualifiers from functions since they are now redundant.

159 ↗(On Diff #192623)

Thanks, much cleaner this way.

ilya-biryukov marked an inline comment as done.Apr 1 2019, 10:38 AM

Looks neat, thanks!
A final set of NITs from my side, I don't think any of the comments have to do with the actual design of the library, so expect LGTM in the next round.

clang/include/clang/Tooling/Refactoring/Transformer.h
82 ↗(On Diff #192889)

NIT: Maybe be more specific: string-based binding ids?

125 ↗(On Diff #192889)

NIT: bound to this id

136 ↗(On Diff #192889)

NIT: Maybe remove the constructor and let the builder handle this?
Technically, users can break the invariants after creating the rewrite rules anyway, so having this constructor does not buy much in terms of safer coding practices, but makes it harder to use RewriteRule as a plain struct (specifically, having no default constructor might make it awkward).

Up to you, of course.

158 ↗(On Diff #192889)

NIT: return by value to make the signatures less clunky. RVO should insure the generated code is the same in practice.
Bonus point: that'd mean we don't need && qualifier on the builder functions too, only at consume().

184 ↗(On Diff #192889)

Isn't this overload redundant in presence of buildRule(DynTypedMatcher)? Both seem to call into the constructor that accept a DynTypedMatcher, so Matcher<T> is convertible to DynTypedMatcher, right?.

70 ↗(On Diff #192623)

Up to you, to me it feels like the presence of this field defines what this class is used for.

  1. If there's an explanation, it feels like it should represent a complete fix or refactoring that could be presented to the user.
  2. Without an explanation, it might feel like something lower-level (e.g. one could write a bunch of RewriteRule whose changes are later combined and surfaced to the user as a full refactoring).

Both approaches make sense, and I assume (1) is the desired abstraction this class represents, so keeping the field looks ok.

89 ↗(On Diff #192623)

Yeah, exactly, but could we keep is a bit shorter by removing the pieces which are non-relevant to the actual transformer usage.
Something like:

// To apply a rule to the clang AST, use Transformer class:
// \code
//   AtomicChanges Changes;
//   Transformer T(
//       MyRule, [&Changes](const AtomicChange &C) { Changes.push_back(C); };);
//   ast_matchers::MatchFinder MatchFinder;
//   T.registerMatchers(&MatchFinder);
//   // ... insert code to run the ast matchers.
//   // Consume the changes.
//   applyAtomicChanges(..., Changes);

Or just mention that Transformer class should be used to apply the rewrite rule and obtain the corresponding replacements.

clang/lib/Tooling/Refactoring/Transformer.cpp
133 ↗(On Diff #192623)

Yeah, you should explicitly qualify to let the compiler know the namespace of a corresponding declaration:

Expected<Transformation>
clang::tooling::applyRewriteRule(...) {
  // ...
}

tooling::applyRewriteRule would also work since we have using namespace clang::tooling; at the top of the file.

157 ↗(On Diff #192623)

The current approach LG, and we have the comments about those cases.

I agree with you that handling macros is hard, so skipping them makes sense for most rewrite rules one would write and at the same time, it'd be useful to let the users of the code know that a rewrite rule could not be applied because of macros.
Returning an empty transformation in that case seems like the best option, the alternative that I can think is less nice: we could have a special error for macros and make users llvm::handleErrors to figure out those cases. This is clunky and the resulting code is not very nice.

clang/unittests/Tooling/TransformerTest.cpp
61 ↗(On Diff #192623)

LG.
As a note, my colleagues mentioned that the LLVM Style Guide says one should put 'static' even in that case, because it makes it easier for readers of the code to spot the file-private functions (they don't need to look for an enclosing anonymous namespace).

I'm personally happy either with or without static.

ymandel updated this revision to Diff 193219.Apr 1 2019, 6:49 PM
ymandel marked 28 inline comments as done.

Assorted changes in response to comments.

Most notably, dropped constructor from RewriteRule, in favor of putting meaningful initialization in the builder.

ymandel added inline comments.Apr 1 2019, 6:50 PM
clang/include/clang/Tooling/Refactoring/Transformer.h
136 ↗(On Diff #192889)

Agreed, but DynTypedMatcher has no default constructor, so we have to provide something. I dropped the constructor, but added an initializer to Matcher to enable the default constructor.

184 ↗(On Diff #192889)

I think I was avoiding some extra construction, but even if so, I can't see its worth the added complexity. thx

70 ↗(On Diff #192623)

Agreed. Keeping it.

89 ↗(On Diff #192623)

went w/ the second suggestion.

clang/lib/Tooling/Refactoring/Transformer.cpp
133 ↗(On Diff #192623)

ah...

157 ↗(On Diff #192623)

Agreed. If error handling was nicer, then I think that your suggestion would be ideal.

clang/unittests/Tooling/TransformerTest.cpp
61 ↗(On Diff #192623)

static sounds good to me.

ilya-biryukov accepted this revision.Apr 2 2019, 1:00 AM

Looks great, thanks!

clang/include/clang/Tooling/Refactoring/Transformer.h
110 ↗(On Diff #193219)

NIT: llvm::StringLiteral is a vocabulary type with compile-time size that could be used here.
Although I don't think it has any actual benefits over char array, so leaving as is also looks good.

113 ↗(On Diff #193219)

NIT: comma seems redundant, this should probably be: A fluent builder class

This revision is now accepted and ready to land.Apr 2 2019, 1:00 AM
ymandel updated this revision to Diff 193260.Apr 2 2019, 5:41 AM
ymandel marked 3 inline comments as done.

Small tweaks in response to comments.

Thank you for the extremely helpful and detailed review!!

clang/include/clang/Tooling/Refactoring/Transformer.h
110 ↗(On Diff #193219)

Went with StringLiteral -- given that we always wrap RootId in a stringref to use it, seems better just to define it that way to begin with.

ymandel updated this revision to Diff 193306.Apr 2 2019, 8:53 AM

Remove dependency on NodeIds.

I've removed the dependency on NodeIds given our (off thread) discussions regarding their overall value and appropriateness in clang::tooling vs clang::ast_matchers. PTAL.

ilya-biryukov accepted this revision.Apr 3 2019, 12:55 AM

LGTM with the new changes. Specifying the clang::Expr type explicitly when calling change looks a bit less clear than the original approach, but also looks pretty clear to me.

This revision was automatically updated to reflect the committed changes.
ABataev added a subscriber: ABataev.Apr 3 2019, 9:42 AM

Patch breaks the build with the shared libraries, for example, http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/9498. Seems to me, you need to add an extra dependency on clangTooling. clangToolingCore is not enough.

You need to provide -DBUILD_SHARED_LIBS=true to cmake. Unfortunately, there is a dependency from the libToolingRefactor to libTooling. I don't think it will be easy to fix this.

ymandel reopened this revision.Apr 3 2019, 12:59 PM

Reopening to fix the breakage caused by previous attempt at commit.

This revision is now accepted and ready to land.Apr 3 2019, 12:59 PM
ABataev removed a subscriber: ABataev.Apr 3 2019, 1:00 PM
ymandel updated this revision to Diff 193584.Apr 3 2019, 1:36 PM
ymandel added a subscriber: ABataev.

Sever dependency of clangToolingRefactor on clangTooling via FixIt.

Transformer used FixIt, which causes a problematic dependency. This diff copies
the (minimal) code from FixIt to Transformer.cpp to break the dependency. For
the future, though, we should consider whether the FixIt library should live
somewhere accessible to components in Refactoring (perhaps Core?).

Ilya, this diff fixes the breakage. PTAL.

ilya-biryukov added a comment.EditedApr 4 2019, 2:23 AM

Given that we don't any usages of getExtendedRange, could you remove it from the Fixit.h and move to a header somewhere in the clangToolingRefactoring (e.g. into Transformer.h itself)?

Having two copies of this functions in the codebase does not look good, especially given that one of them is never used anyway.

Keeping it only in the cpp file also LG if we don't plan to have other usages for now, but please remove the version from FixIt.h

Keeping it only in the cpp file also LG if we don't plan to have other usages for now, but please remove the version from FixIt.h

The Stencil library will need this as well and I've heard from another user who's already using this (a standalone tool, not in clang), so it seems to have a general value. Might the fixit library belong in Core? I have a whole bunch more utility functions coming along, so we need a place for them as well.

I propose that I create a new library in Core with getExtendedRange() and remove it from FixIt. The other utility functions that I need will also go there. We can separately investigate moving the remaining pieces of FixIt into Core.

I propose that I create a new library in Core with getExtendedRange() and remove it from FixIt. The other utility functions that I need will also go there. We can separately investigate moving the remaining pieces of FixIt into Core.

Moving this to Core makes sense to me, the only concern that I have is that it's actually a small and focused library as it stands today and adding utility functions to it would seem to cause some chaos.
But it's fine as long as the maintainers of the library are ok with this. I'll ask around to find out who's actually responsible for the library and get back to you...

Per @ioeric's suggestion: why not move the helper into Tooling/Refactoring/ExtendedRange.h?
If it's in ToolingRefactoring, both stencil and transformer can access it.

For external users, a dependency on either ToolingCore or ToolingRefactoring should be fine, since they're fine with a dependency on Tooling already.

Per @ioeric's suggestion: why not move the helper into Tooling/Refactoring/ExtendedRange.h?
If it's in ToolingRefactoring, both stencil and transformer can access it.

For external users, a dependency on either ToolingCore or ToolingRefactoring should be fine, since they're fine with a dependency on Tooling already.

This sounds perfect. Can I do the same for the future additions -- small, focused libraries for each group of functions? I just want to be sure that we don't regret the name "ExtendedRange" when I need to add the next batch.

ioeric added a comment.Apr 4 2019, 7:12 AM

Per @ioeric's suggestion: why not move the helper into Tooling/Refactoring/ExtendedRange.h?
If it's in ToolingRefactoring, both stencil and transformer can access it.

For external users, a dependency on either ToolingCore or ToolingRefactoring should be fine, since they're fine with a dependency on Tooling already.

This sounds perfect. Can I do the same for the future additions -- small, focused libraries for each group of functions? I just want to be sure that we don't regret the name "ExtendedRange" when I need to add the next batch.

In clangd, we have a library called SourceCode.h that keeps source code related helpers including those that deal with ranges. I think it's reasonable to use SourceCode.h or something similar here.

ymandel updated this revision to Diff 193748.Apr 4 2019, 10:47 AM

Switch from using FixIt to SourceCode, as solution to circular dependency problem.

ymandel updated this revision to Diff 193750.Apr 4 2019, 10:49 AM

Rebasing to parent D60269 so that diffs show correctly.

ilya-biryukov accepted this revision.Apr 5 2019, 2:42 AM

One more LGTM.

ymandel updated this revision to Diff 193869.Apr 5 2019, 7:15 AM

Rebase onto master

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2019, 8:12 AM