=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>, =?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>, =?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>, =?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>, =?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>, =?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>,Balazs Benics <benicsbal...@gmail.com>,Balazs Benics <benicsbal...@gmail.com>, =?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>, =?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/139...@github.com>
https://github.com/steakhal commented: Thank you for the detailed answer to my previous reply. I think we are completely aligned on that front. Thank you for accepting my patches, and also for implementing exactly what I had in mind for the debug names. It's awesome. I don't think it's too complicated or ugly TBH. I agree it's not nice, but we can either merge this or discuss how else we could do something even better. I'm happy as this PR looks right now, except for having that backward compatibility overload for plugins. We can just move on with life and let them migrate once the next clang is out. To me, the upgrade path looks straight forward and this isn't the only API they will need to adjust anyway. They should brace for impact. Now, to the alternative options you layed out. (Thanks for thinking about this btw.) (PS: Sorry about my seemingly unstructured response here to the list. To me, these options are not orthogonal and it makes it difficult to reflect.) I'm fine with option 6, aka. just merge this PR as-is. > 6. Use the `CLASS` field of the checker specifications in `Checkers.td` (as > currently implemented by 13f4a30) > Very misleading, looks like a nice proper identifier (the class name) but may > be different in rare cases. > Requires touching the fragile "register checkers from analyzer plugins" logic. > "Identifier" is not stable: depends on the set of currently registered > subcheckers. Could you give an example when it's misleading and not refers to a class name? I figured the tablegen construct we have ensures this. We should be fine touching the plugin registration. This shouldn't be disruptive. WDYM by that this identifier is not stable? We could have a tablegen property like `ImplementedBy<"MyChecker">` for the few checker families we currently have, and say that by default they are implemented by `CLASS`. I reject options 1 and 2, as we already discussed. I agree with your statement in option 3 about the identifier of a class. And I think I already layed out something like this part of the previous section in this reply. I'm fine with option 4, but I want to avoid this if possible. I reject option 5, as I find it a buggy implementation of option 6 (or option 4). https://github.com/llvm/llvm-project/pull/139256 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
