-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[clang][NFC] Refactor replaceExternalDecls to use llvm::any_of #143275
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
[clang][NFC] Refactor replaceExternalDecls to use llvm::any_of #143275
Conversation
@llvm/pr-subscribers-clang Author: Samarth Narang (snarang181) ChangesThis patch simplifies the declaration replacement logic in replaceExternalDecls by replacing a manual loop with an idiomatic use of llvm::any_of. This improves code readability and aligns with common LLVM coding style. Full diff: https://github.com/llvm/llvm-project/pull/143275.diff 1 Files Affected:
diff --git a/clang/include/clang/AST/DeclContextInternals.h b/clang/include/clang/AST/DeclContextInternals.h
index b17b7627ac90c..a1071f62d5fae 100644
--- a/clang/include/clang/AST/DeclContextInternals.h
+++ b/clang/include/clang/AST/DeclContextInternals.h
@@ -176,14 +176,10 @@ class StoredDeclsList {
DeclListNode::Decls *Tail = erase_if([Decls](NamedDecl *ND) {
if (ND->isFromASTFile())
return true;
- // FIXME: Can we get rid of this loop completely?
- for (NamedDecl *D : Decls)
- // Only replace the local declaration if the external declaration has
- // higher visibilities.
- if (D->getModuleOwnershipKind() <= ND->getModuleOwnershipKind() &&
- D->declarationReplaces(ND, /*IsKnownNewer=*/false))
- return true;
- return false;
+ return llvm::any_of(Decls, [ND](NamedDecl *D) {
+ return D->getModuleOwnershipKind() <= ND->getModuleOwnershipKind() &&
+ D->declarationReplaces(ND, /*IsKnownNewer=*/false);
+ });
});
// Don't have any pending external decls any more.
|
@ChuanqiXu9, requesting your review here. |
IIUC the |
D->declarationReplaces(ND, /*IsKnownNewer=*/false)) | ||
return true; | ||
return false; | ||
return llvm::any_of(Decls, [ND](NamedDecl *D) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remain the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the comment back. Is that what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We should keep both.
dc4e3c9
to
8446545
Compare
OK. In any case, the usage of |
@@ -177,13 +177,10 @@ class StoredDeclsList { | |||
if (ND->isFromASTFile()) | |||
return true; | |||
// FIXME: Can we get rid of this loop completely? | |||
for (NamedDecl *D : Decls) | |||
// Only replace the local declaration if the external declaration has | |||
// higher visibilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is another missing comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Should we keep the FIXME?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retain all now, I think.
9bee4e3
to
ff09e5c
Compare
ff09e5c
to
f849821
Compare
This patch simplifies the declaration replacement logic in replaceExternalDecls by replacing a manual loop with an idiomatic use of llvm::any_of. This improves code readability and aligns with common LLVM coding style.