ideasman42 wrote: > 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?
I would not depend on it - instead, it would be loosely coupled - if `clang-format-vc-diff-function` was set, it would be used. Otherwise `clang-format-vc-diff` wouldn't work (reporting the function to calculate changed lines needs to be set). --- > 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. This can happen for intricate API's - in this case - a single function that returns line-ranges: `(list (int . int) ...)` is a fairly minimal interface, so I don't see separating the code out as adding overhead/burden. ---- > 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. Fair enough - as noted, this is fairly opinionated - and I don't think your "wrong" for holding the contrary position. Keep in mind many people have been using `clang-format.el` for years without this functionality, and will continue to do so. I disagree that this change is simple though, it seems simple on face value - based on the review - using git & diff with their different versions ... possible optimizations, possible errors when they fail ... is in fact more complicated than may first seem. There are some potential bugs that could bite us: - How to handle git failing (if git doesn't know about the file... has a corrupt repository... the file could be in the middle of resolving a conflict). - What if git or diff considers the file to be a binary file. - The emacs buffers should use the encoding settings from the buffer that is being edited... do they? (I'd need to double check - at a guess - they don't seem to at the moment). This is not to attempt to shoot down the contribution, just to note that there are all sorts of cases where things can go wrong - where a feature like this turns out not to be all that simple and there are corner cases that need to be investigated/resolved. As noted, this is my suggestion to support a customizable line-range generator so `clang-format.el can be kept minimal. I'll leave the decision to others. 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
