[PATCH] D61015: [LibTooling] Change Transformer's TextGenerator to a partial function.

[Ilya Biryukov via Phabricator via cfe-commits]

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