erichkeane wrote: > @erichkeane > > > > _should all errors/warnings have explicit stable IDs?_ > > > > > > 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%. > > OK, let's do that. A couple details: First, what's the best way to ensure > that a developer changing or adding a diagnostic understands that the stable > ID needs to be stable? Using the `StableId<>` class from my tblgen PR makes > it more obvious, rather than just adding a second unnamed parameter to the > `Warning<>` and `Error<>` classes. Does that sound OK? > Yep, agreed. I suspect changing `StableId` is something that any human programmer would think twice about, and hopefully any reviewer would notice. I think an implicit value is likely to not do a good job here.
> Second, can I add those explicit stable IDs in a follow-up PR? So the PR > sequence would be something like: > > 1. The current PR (implicit stable IDs) > > 2. Add tblgen support for (optional) explicit stable IDs [PR > here](https://github.com/dbartol/llvm-project/pull/2) > > 3. Add explicit stable IDs for all warnings and errors. > > 4. Make explicit stable IDs required for warnings and errors. > I think a better order: Add support for `StableId`, add explicit IDs, make explicit `StableId` required for warnings/errors. I don't see any value to the 'implicit' version in the meantime. > > I'm kind of dreading when PRs 3 and 4 hit my (or anyone else's) downstream > fork, but I think having them as separate PRs will make that easier. It would > be possible to merge PR 3 (which will probably have some merge conflicts), > then go add stable IDs to all the downstream-only diagnostics, then merge PR > 4. Yes, downstreams are going to have some pain here :) Perhaps share the script you use for the explicit stable-id values/adding them would be a friendly thing to do, but that is the risk of being a downstream :) https://github.com/llvm/llvm-project/pull/168153 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
