erichkeane wrote: > @AaronBallman @erichkeane > > Both of you make good arguments for keeping error IDs stable, so I'm on board > with that. I don't think that decision changes this PR or the tblgen PR I > linked to, since the new properties are supported on any `Diagnostic` object > already. It does change the hypothetical enforcement bot, though. > > > I think I'd be ok making Tablegen enforce adding a new 'tag' to each > > diagnostic that has its StableId. Not sure I like the name OldStableIds, > > but that is bikeshedding. You'd just have to do an initial population of > > that, and a script to name them.... something would be acceptable. > > If the initial StableId tag is just auto-generated from the existing > definition of the diagnostic, is there any point in actually adding all those > initial StableId tags to the `.td` files? Even if we add these, we'd still > need some kind of PR-time (not build-time, see below) enforcement to spot > devs renaming/splitting/merging diagnostics and remind them to update > OldStableIds. Having all StableIds be explicit might make it more likely for > a dev who renames a diagnostic to keep its stable ID the same, but they'd > still probably need the PR bot to remind them to update OldStableIds.
Yes, there IS. This removes an entire class of 'rename problems' (AND the most likely ones!) to the point that PR-time enforcement will catch orders-of-magnitude fewer things. OF your list above, 96 + 15 + 240 all seem like 'not a problem'. This would enforce that breaking the SARIF name is an actual, active, intentional thing, which would remove all of the 13. I would still want periodic checking for these, but it would remove our risk of breaking them by almost 100%. > > > I think this approach is a little subtle. And unless we have a way of > > enforcing when this happened, it'll be missed. IMO, we should just bite the > > bullet, require StableId via tablegen, and just come up iwth a heuristic > > for the 'initial set' of errors+ warnings. > > ... > > I'd rather tablegen enforce the existence, a CI job/etc to do it would be > > acceptable. > > Tblgen can only enforce rules that only depend on the current version of the > `.td` files. For example, we can and should enforce a "no duplicate > StableIds" rule in tblgen at build time. The PR bot would be needed to catch > cases where someone renamed a diagnostic and didn't keep the StableId > (implicit or explicit) stable. It would also be needed to catch the more > complex "merged two diagnostics" or "split a diagnostic" cases. Those cases > require looking at the `.td` files before and after the change. Right, I'm aware. Bur enforcing that we have those initial values means that the 'implicit' names don't cause an accidental breakage, and catch errors 'earlier' (even before the person submits their patch!). IMO, this will result in a much more stable set of names with less work on our parts. https://github.com/llvm/llvm-project/pull/168153 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
