[ADT] Fix MSVC build after iterator C++20 fix #173495
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-llvm-adt Author: Jorn Tuyls (jtuyls) ChangesFixes an MSCV build issue after the C++20 fix in #169772. See the failure log in the IREE downstream project. Making IsRandomAccess, IsBidirectional public ensures that they are always accessible, avoiding the access-related SFINAE ambiguity that causes different compilers to handle this differently. The build is passing after this change: https://github.com/iree-org/iree/actions/runs/20485132054/job/58865989220?pr=22979 Full diff: https://github.com/llvm/llvm-project/pull/173495.diff 1 Files Affected:
diff --git a/llvm/include/llvm/ADT/iterator.h b/llvm/include/llvm/ADT/iterator.h
index c0495e24893fb..7444409508b53 100644
--- a/llvm/include/llvm/ADT/iterator.h
+++ b/llvm/include/llvm/ADT/iterator.h
@@ -85,7 +85,9 @@ class iterator_facade_base {
using pointer = PointerT;
using reference = ReferenceT;
-protected:
+ // Note: These were previously protected, but MSVC has trouble with SFINAE
+ // accessing protected members in derived class templates (specifically in
+ // iterator_adaptor_base::operator-). Making them public fixes the build.
enum {
IsRandomAccess = std::is_base_of<std::random_access_iterator_tag,
IteratorCategoryT>::value,
@@ -93,6 +95,8 @@ class iterator_facade_base {
IteratorCategoryT>::value,
};
+protected:
+
/// A proxy object for computing a reference via indirecting a copy of an
/// iterator. This is used in APIs which need to produce a reference via
/// indirection but for which the iterator object might be a temporary. The
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
jtuyls
force-pushed
the
fix-adt-msvc-error
branch
from
977596b to
ddcb06b
Compare
Contributor
Author
|
Cc some of the participants in #169772: @vedranmiletic, @kuhar, @cor3ntin, @jwakely. Could you help review? |
Contributor
|
Can you post the failure log here? It is not accessible without authentication. |
kuhar
approved these changes
Dec 24, 2025
Contributor
jtuyls
added a commit
to iree-org/iree
that referenced
this pull request
Dec 26, 2025
Integrate llvm/llvm-project@c0f4a8a Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. Also adds a revert for a commit that breaks the MSVC build: * Local revert of llvm/llvm-project#169772. This can be dropped after llvm/llvm-project#173495. Signed-off-by: Jorn Tuyls <[email protected]>
Contributor
Author
That also works, but we need the forward declaration of Here is the diff for the friend solution: Also, here is a snippet of the failure log btw: |
Contributor
It is a bit weird, but OTOH making them public is an API promise, which might be harder to change in the future. I would say the comment explains the weirdness of MSVC and would actually lean towards that solution. |
Member
|
ADT is not considered stable API |
Contributor
|
Right, then making the symbols public is fine and definitely simpler. Sorry, I don't have GitHub access to formally approve. |
Contributor
Author
|
If ok, could someone help merge this? @kuhar @vedranmiletic |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/35595 Here is the relevant piece of the build log for the reference |
jtuyls
added a commit
to iree-org/iree
that referenced
this pull request
Dec 29, 2025
Integrate llvm/llvm-project@75a0347 Removes a revert: * Local revert of llvm/llvm-project#169772 as the fix in llvm/llvm-project#173495 got merged. Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. Signed-off-by: Jorn Tuyls <[email protected]>
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Jan 6, 2026
Fixes an MSCV build issue after the C++20 fix in llvm#169772. See the [failure log](https://productionresultssa1.blob.core.windows.net/actions-results/604d315e-edbd-401f-9a85-9ec5fcbc4996/workflow-job-run-99b94847-47a4-5b95-9933-44db3e32a2a7/logs/job/job-logs.txt?rsct=text%2Fplain&se=2025-12-24T11%3A16%3A19Z&sig=3leOtxGMlJmAMzOCtakzD8%2FOQCXF2HfflooR%2Bm%2Bt7Ng%3D&ske=2025-12-24T21%3A53%3A06Z&skoid=ca7593d4-ee42-46cd-af88-8b886a2f84eb&sks=b&skt=2025-12-24T09%3A53%3A06Z&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skv=2025-11-05&sp=r&spr=https&sr=b&st=2025-12-24T11%3A06%3A14Z&sv=2025-11-05) in the IREE downstream project. Making IsRandomAccess, IsBidirectional public ensures that they are always accessible, avoiding the access-related SFINAE ambiguity that causes different compilers to handle this differently. The build is passing after this change: https://github.com/iree-org/iree/actions/runs/20485132054/job/58865989220?pr=22979
keshavvinayak01
pushed a commit
to iree-org/iree
that referenced
this pull request
Jan 27, 2026
Integrate llvm/llvm-project@c0f4a8a Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. Also adds a revert for a commit that breaks the MSVC build: * Local revert of llvm/llvm-project#169772. This can be dropped after llvm/llvm-project#173495. Signed-off-by: Jorn Tuyls <[email protected]> Signed-off-by: Keshav Vinayak Jha <[email protected]>
keshavvinayak01
pushed a commit
to iree-org/iree
that referenced
this pull request
Jan 27, 2026
Integrate llvm/llvm-project@75a0347 Removes a revert: * Local revert of llvm/llvm-project#169772 as the fix in llvm/llvm-project#173495 got merged. Existing local reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. Signed-off-by: Jorn Tuyls <[email protected]> Signed-off-by: Keshav Vinayak Jha <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes an MSCV build issue after the C++20 fix in #169772. See the failure log in the IREE downstream project.
Making IsRandomAccess, IsBidirectional public ensures that they are always accessible, avoiding the access-related SFINAE ambiguity that causes different compilers to handle this differently.
The build is passing after this change: https://github.com/iree-org/iree/actions/runs/20485132054/job/58865989220?pr=22979