https://github.com/NagyDonat requested changes to this pull request.
As a maintainer of the static analyzer, I'm opposed to this change. I understand that you sincerely wanted to help, but this is not a helpful contribution. First I think this is an appropriate place to quote "premature optimization is the root of all evil" from Donald Knuth. There _might be_ one or two call sites in deeply nested loops where using `printQualifiedName` instead of `getQualifiedNameAsString` has a measurable performance impact – but even then, the bulk of this huge commit (600 added source lines!) has no advantages. (Personally I'd guess that the overall performance impact is negligible, because most calls to `getQualifiedNameAsString` will be printed to the user and the overall size of the output is not too large.) There are also several other problems: - In many files this commit just duplicates the implementation of `getQualifiedNameAsString` without any performance advantage. Those changes are definitely unjustified – and show that `getQualifiedNameAsString` needs to be preserved even if you - The PR also includes changes (like the one that I marked with an inline comment) that are completely unrelated to its stated goal. Did you intentionally include these, or is this part of AI output that you failed to sanitize? - Avoid tree-wide changes unless they are absolutely necessary because they affect many reviewers from different areas. - For example if you want to phase out a function, you should create separate PRs that handle each area independently. - Instead of publishing all changes once, the first few PRs should be created sequentially, to ensure that you learn from the first reviews and won't make the same mistakes in the subsequent PRs. - I know that creating one big PR is easier for you, but this is extractive "contribution" that wastes the time of the reviewers. Also note that you should not blindly trust `FIXME` and `TODO` notes in the LLVM codebase, especially if they are old. Many such comments contain outdated or just contraproductive ideas and you should question their justification before following them. (This is a fault of the codebase, and it was reasonable that you trusted the `FIXME` note – but please do not do so next time.) I personally suggest abandoning this PR and creating a new single-line PR which just removes the `FIXME` line. https://github.com/llvm/llvm-project/pull/218264 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
