ilya-biryukov added inline comments.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:47
-using TextGenerator =
- std::function<std::string(const ast_matchers::MatchFinder::MatchResult &)>;
+// \c TextGenerator may fail, because it processes dynamically-bound match
+// results. For example, a typo in the name of a bound node or a mismatch in
----------------
NIT: maybe shorten a bit, still capturing the essence? Something like the
following should be enough:
```
Note that \p TextGenerator is allowed to fail, e.g. when trying to access a
matched node that was not bound.
Allowing this to fail simplifies error handling for interactive tools like
clang-query.
```
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:58
-/// Wraps a string as a TextGenerator.
+/// Wraps a string as a (trivially successful) TextGenerator.
inline TextGenerator text(std::string M) {
----------------
Maybe drop `trivially successful`? Does not seem to be super-important.
================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:238
+ /// occurs in constructing a single \c AtomicChange then the change is still
+ /// passed to \c Consumer, but it's error will be set. Note that clients are
+ /// responsible for handling the case that independent \c AtomicChanges
----------------
s/it's/its
================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:164
return SmallVector<Transformation, 0>();
- T.Replacement = Edit.Replacement(Result);
+ auto ReplacementOrErr = Edit.Replacement(Result);
+ if (auto Err = ReplacementOrErr.takeError())
----------------
Maybe follow a typical pattern for handling errors here (to avoid `OrErr`
suffixes and an extra `Err` variable)? I.e.
```
auto Replacement = Edit.Replacement(Result);
if (!Replacement)
return Replacement.takeError();
T.Replacement = std::move(*Replacement);
```
================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:205
if (auto Err = TransformationsOrErr.takeError()) {
- llvm::errs() << "Transformation failed: " << llvm::toString(std::move(Err))
- << "\n";
+ AC.setError(llvm::toString(std::move(Err)));
+ Consumer(AC);
----------------
This looks super-complicated.
Having `Error` in `AtomicChange` seems like a bad idea in the first place, why
would we choose to use it here?
The following alternatives would encourage clients to handle errors properly:
1. accept an `Expected<AtomicChange>` in our callback,
2. provide a separate callback to consume errors.
WDYT about picking one of those two?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61015/new/
https://reviews.llvm.org/D61015
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
