ilya-biryukov added inline comments.
================ Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:77 // - if apply() returns an error, returns "fail: <message>" - std::string apply(llvm::StringRef MarkedCode) const; + std::string apply(llvm::StringRef MarkedCode); ---------------- kadircet wrote: > ilya-biryukov wrote: > > It seems weird that we have this inconsistency between the contents for > > current file (passed through the return value) and contents for other files > > (pass through the fields). > > > > A few better alternatives that come to mind: > > 1. add an out parameter, could be optional in case when all changes are in > > the main file. > > 2. this function will fail when there are changes outside main file, but we > > can add a new function to return changes in all modified files, e.g. as > > `StringMap<std::string>` or `vector<pair<Path, string/*Content*/>>` (the > > latter allows to use standard matchers) > It is left-out this way since most of the checks only care about a single > file, and there are lots of them so changing would definitely require some > plumbing. > > I am not sure having the inconsistency between main file and others matters > so much though; we have similar discrimination towards non-main files in a > bunch of other places in testing infrastructure e.g TestTU. > > As for the suggestions, adding another apply function for multifile case > doesn't seem so nice, so I would rather move forward with first one, if you > believe this is a strong concern. > It is left-out this way since most of the checks only care about a single > file, and there are lots of them so changing would definitely require some > plumbing. Agree that no matter what we do, we should let the current usages stay the same. > I am not sure having the inconsistency between main file and others matters > so much though; we have similar discrimination towards non-main files in a > bunch of other places in testing infrastructure e.g TestTU. What kind of inconsistency are you referring to? Both main file and extra files are fields inside `TestTU`? It outputs `ParsedAST` and it's a return value of the `build()` function. There are no multiple ways to return results. > As for the suggestions, adding another apply function for multifile case > doesn't seem so nice, so I would rather move forward with first one, if you > believe this is a strong concern. Yeah, totally ok with this. I'm not a big fan of out parameters myself and prefer putting stuff into the return value instead. But they're still widely used, so I view this as my preference rather than a common convention (sigh...) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647/new/ https://reviews.llvm.org/D66647 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
