dbartol 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. >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. https://github.com/llvm/llvm-project/pull/168153 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
