On Thu, Nov 8, 2018 at 8:56 PM Martin Ågren <martin.ag...@gmail.com> wrote: > I haven't followed the original discussion too carefully, so I'll read > this like someone new to the topic probably would.
Thanks! > A nit, perhaps, but I was genuinely confused at first. The subject is > "Makefile: add pending semantic patches", but this patch doesn't add > any. It adds Makefile-support for such patches though, and it defines > the entire concept of pending semantic patches. How about "coccicheck: > introduce 'pending' semantic patches"? > > > Add a description and place on how to use coccinelle for large refactorings > > that happen only once. > > A bit confused about "and place". Based on my understanding from reading > the remainder of this patch, maybe: > > Teach `make coccicheck` to avoid patches named "*.pending.cocci" and > handle them separately in a new `make coccicheck-pending` instead. > This means that we can separate "critical" patches from "FYI" patches. > The former target can continue causing Travis to fail its static > analysis job, while the latter can let us keep an eye on ongoing > (pending) transitions without them causing too much fallout. > > Document the intended use-cases and processes around these two > targets. Both suggested title and new commit message make sense. > > > This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/) > > semantic patches that might be useful to developers. > > + > > +There are two types of semantic patches: > > + > > + * Using the semantic transformation to check for bad patterns in the code; > > + This is what the original target 'make coccicheck' is designed to do and > > Is it relevant that this was the "original" target? (Genuine question.) No. I can omit that part. > > > + it is expected that any resulting patch indicates a regression. > > + The patches resulting from 'make coccicheck' are small and infrequent, > > + so once they are found, they can be sent to the mailing list as per > > usual. > > + > > + Example for introducing new patterns: > > + 67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28) > > + b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24) > > + > > + Example of fixes using this approach: > > + 248f66ed8e (run-command: use strbuf_addstr() for adding a string to > > + a strbuf, 2018-03-25) > > + f919ffebed (Use MOVE_ARRAY, 2018-01-22) > > + > > + These types of semantic patches are usually part of testing, c.f. > > + 0860a7641b (travis-ci: fail if Coccinelle static analysis found > > something > > + to transform, 2018-07-23) > > Very nicely described, nice with the examples to quickly give a feeling > about how/when to use this. Thanks. > > > + * Using semantic transformations in large scale refactorings throughout > > + the code base. > > + > > + When applying the semantic patch into a real patch, sending it to the > > + mailing list in the usual way, such a patch would be expected to have a > > + lot of textual and semantic conflicts as such large scale refactorings > > + change function signatures that are used widely in the code base. > > + A textual conflict would arise if surrounding code near any call of such > > + function changes. A semantic conflict arises when other patch series in > > + flight introduce calls to such functions. > > OK, I'm with you. > > > + So to aid these large scale refactorings, semantic patches can be used, > > + using the process as follows: > > + > > + 1) Figure out what kind of large scale refactoring we need > > + -> This is usually done by another unrelated series. > > "This"? The figuring out, or the refactoring? Also, "unrelated"? The need and type of what kind of large scale refactoring are usually determined by a patch series, that is not refactoring for the sake of refactoring, but it wants to achieve a specific goal, unrelated to large refactorings per se. The large refactoring is just another tool that a developer can use to make their original series happen much faster. So "unrelated" == "not the large scale refactoring, as that may come as an preparatory series, but to have a preparatory series it may be good to showcase why we need the preparatory series" > > > + 2) Create the sematic patch needed for the large scale refactoring > > s/sematic/semantic/ yup. > > > + and store it in contrib/coccinelle/*.pending.cocci > > + -> The suffix containing 'pending' is important to differentiate > > + this case from the other use case of checking for bad patterns. > > Good. > > > + 3) Apply the semantic patch only partially, where needed for the patch > > series > > + that motivates the large scale refactoring and then build that series > > + on top of it. > > + By applying the semantic patch only partially where needed, the > > series > > + is less likely to conflict with other series in flight. > > + To make it possible to apply the semantic patch partially, there > > needs > > + to be mechanism for backwards compatibility to keep those places > > working > > + where the semantic patch is not applied. This can be done via a > > + wrapper function that has the exact name and signature as the > > function > > + to be changed. > > + > > + 4) Send the series as usual, including only the needed parts of the > > + large scale refactoring > > Trailing period. ok. > OK, I think I get it, but I wonder if this might not work equally well > or better under certain circumstances: > > - introduce new API The "new API" is similar enough to the old API and may even be a superset. > - add pending semantic patch > - convert quiet areas to use the new API > > On the other hand, listing all possible flows might be needlessly > limiting. I guess it boils down to this: > > "Create a pending semantic patch. Make sure the old way of doing things > still works. Apply the semantic patch to the quieter areas of the > codebase. If in doubt, convert fewer places in the original series -- > remaining spots can always be converted at a later time." That's the gist of it. :) Thanks for the review, Stefan
