dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land.
In D104777#2844077 <https://reviews.llvm.org/D104777#2844077>, @brunodefraine wrote: > That I already encounter this in builtin headers is probably an indication > that the new error will break some code in the wild. Fair point, breaking backwards compatibility isn't great. > A pragmatic solution would be to only raise the error in > `Sema::mergeDeclAttributes` if `Old->isUsed()`, i.e. an earlier declaration > without `nodebug` can be forgiven if not yet used. I think that would still > make it impossible to trigger the crash? I'm not too fussed about the original patch - just figured it might be nicer/cleaner to require a consistent view of the attribute value. I'm not sure it's worth splitting hairs on "it can be inconsistent but only in this narrow way". I can appreciate that only invalidating the cases that were crashes before avoids increasing the feature surface area more than necessary - but I'm not sure it makes for a more ergonomic feature. (though, equally - the idea that adding a definition to a translation unit would modify the DWARF produced for calls to the function that were previously fine is weird too - but I guess we already had that property if you added the definition at the start of the translation unit) Let's just go with the original patch then - thanks for your patience/working through the alternatives! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104777/new/ https://reviews.llvm.org/D104777 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
