goldsteinn wrote: > _Suggesting a fairly large change to this PR, although it's quite > opinionated._ > > If I were maintaining `clang-format.el` I'd split out the logic for > generating a list of changes into it's own package. > > Exactly how this is done is not be so important... it could for example > support a custom function that generates a list of line number pairs. > > `clang-format-vc-diff` could depend on a custom function which defaults to > nil, and would warn when the function wasn't set. e.g. `(defcustom > clang-format-vc-diff-function ...)` > > The function can simply return an ordered list of integer pairs representing > lines. > > Then there can be a package on MELPA `clang-format-diff` or similar and all > the issues with platform compatibility can be handled there without pull > requests to LLVM. >
So IIUC, you are proposing a create an entirely seperate project for "vc-diff-lines" or something like that then making that a dependency for for `clang-format`? Is that correct? > There are a few reasons this has benefits. > > * The current PR misses `(require ...)` for, `vc` `vc-git` ... however > requiring this is unnecessary for users who wont use the functionality. > > * Any bugs relating to details of different versions of git/diff/WIN32 > compatibility can be handled without going via LLVM PR. > > * Support for other version control or other ways of generating change > ranges can be handled even user-customized - integrated with other packages > that already track changes - such as `diff-hl` could be used. > > * All the details of integrating diff-hl/other-version-control, platform > support (proper error messages if `diff` or `git` isn't found or encounters > some unknown encoding)... can be handled separately. > > * LLVM's `clang-format.el` remains minimal, users who fully format their > source don't need to install the extra functionality. > > * Less maintenance burden for LLVM. > At least IMO, this is the other way around. Relying on external projects I think typically creates a higher maintainer burden, particularly in this case where I think LLVM would be the only user of the new package, so whenever changes where needed for the diff/vc stuff, it would require coordination with an external project. Further, if the external project got its own users and `clang-format.el`'s needs changed, it could become a lot more difficult to make the changes, as now there are independent users who might expect stability/etc... > > The main down side is users who rely on this behavior need to install an > additional package although I don't see this as a big down-side. Overall, I'm pretty down on this. IMO, the vc/diff functionality is pretty specific to the use-case we have in `clang-format.el` and neither is complex enough to warrant or made more convenient by having in an independent package. I think a similiar argument would imply the `git-clang-format` script we already maintain under LLVM should be extract out (or at least all the commands that call into "git") which seems equally unappealing. https://github.com/llvm/llvm-project/pull/112792 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
