ilya-biryukov added a comment. Ah, had some comments and forgot to send them out before I went on vacation :-(
================ Comment at: clang/include/clang/Tooling/FixIt.h:60 +// future to include more associated text (like comments). +CharSourceRange getSourceRangeAuto(const Stmt &S, ASTContext &Context); + ---------------- ymandel wrote: > ilya-biryukov wrote: > > Do you have alternative names in mind? It would be nice to (1) not mention > > the SourceRange now that we return CharSourceRange, (2) change "auto" to > > something more descriptive. > > > > Was thinking about `getNodeRange()` or `getSpannedRange()`, but that > > completely misses the "auto" part (maybe it's fine, though). > > WDYT? Maybe other ideas? > I completely agree. I went through quite a few iterations on this name and > disliked this one the least. ;) I think you're right, though, that once > we're choosing a different name, the "auto" part doesn't really need to be in > it. I like `getSpannedRange` better than this. Other thoughts: > > getLogicalRange > getExtendedRange > getAssociatedRange > > any preference? `getSpannedRange` and `getAssociatedRange` would be my favorites. Both seem to leave no room for interpretation. ================ Comment at: clang/include/clang/Tooling/FixIt.h:73 +// context. In contrast with \p getText(), this function selects a source range +// "automatically", extracting text that a reader might intuitively associate +// with a node. Currently, only specialized for \p clang::Stmt, where it will ---------------- ymandel wrote: > ilya-biryukov wrote: > > What are other tricky cases you have in mind for the future? > I just assumed that we'd hit more as we dig into them, but, I'm not so sure > now. The two others I can think of offhand are 1) extending to include > associated comments, 2) semicolons after declarations. Commas present a > similar challenge (if a bit simpler) when used in a list (vs. the comma > operator). Are there any other separators in C++? > > At a higher level, it would be nice to align this with your work on tree > transformations. That is, think of these functions as short-term shims to > simulate the behavior we'll ultimately get from that new library. However, it > might be premature to consider those details here. Would it be fair to characterize those as "the text range that you needs to be removed when removing a node"? Trying to come up with a comment that does not rely on the intuition, since that might differ from one reader to the other. Maybe move this comment to `getSourceRangeAuto` and in this function's comment simply mention that we return the text for the node covered with `getSourceRangeAuto`? > At a higher level, it would be nice to align this with your work on tree > transformations. That is, think of these functions as short-term shims to > simulate the behavior we'll ultimately get from that new library. However, it > might be premature to consider those details here. Agree, we can figure it out as soon as the syntax trees are in a good enough shape. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58556/new/ https://reviews.llvm.org/D58556 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
