Page MenuHomePhabricator

[DebugInfo] Fix build when std::vector::iterator is a pointer
ClosedPublic

Authored by orivej on Sep 15 2018, 4:36 PM.

Details

Summary

std::vector::iterator type may be a pointer, then iterator::value_type fails to compile since iterator is not a class, namespace, or enumeration.

Diff Detail

Repository
rL LLVM

Event Timeline

orivej created this revision.Sep 15 2018, 4:36 PM

Without this, the error (abbreviated) is:

lib/CodeGen/AsmPrinter/DwarfDebug.cpp: error: 'llvm::MapVector<>::const_iterator' (aka 'const std::pair<> *') is not a class, namespace, or enumeration

@hans, it would be useful to merge this into LLVM 7.

dblaikie accepted this revision.Sep 15 2018, 8:38 PM

Looks good!

This revision is now accepted and ready to land.Sep 15 2018, 8:38 PM

OK, please commit this into trunk first on my behalf!

kristina added a subscriber: kristina.EditedSep 16 2018, 11:20 AM

Hi, I know it's a minor change but it should be easy to add a test case for it (a unit test would be appropriate here), especially considering this isn't currently causing issues in any of the current builds as far as I can tell. Also while I'm not insisting, a more-descriptive explanation of the change be nice as it currently, just a sentence or two, so it's more clear on the mailing lists. Thank you.

orivej edited the summary of this revision. (Show Details)Sep 16 2018, 2:40 PM

I have slightly extended the description.

I don't think this is possible to test with a unit test, as this requires an integration test with a c++ library those std::vector::iterator is a pointer. libc++ becomes such a library with this diff applied to include/vector:

     typedef typename __base::pointer                 pointer;
     typedef typename __base::const_pointer           const_pointer;
+    typedef typename __base::pointer                 iterator;
+    typedef typename __base::const_pointer           const_iterator;
-    typedef __wrap_iter<pointer>                     iterator;
-    typedef __wrap_iter<const_pointer>               const_iterator;
kristina added a comment.EditedSep 16 2018, 2:56 PM

Alright, I'll just run this on a single buildbot and then land it. (I think it's possible to mock it, I can make a differential for a testcase later myself)

This revision was automatically updated to reflect the committed changes.
hans added a comment.Sep 17 2018, 12:39 AM

Without this, the error (abbreviated) is:

lib/CodeGen/AsmPrinter/DwarfDebug.cpp: error: 'llvm::MapVector<>::const_iterator' (aka 'const std::pair<> *') is not a class, namespace, or enumeration

Do you have an example of a c++ std library where this happens?

@hans, it would be useful to merge this into LLVM 7.

I've put it on my list, but we're currently not planning another release candidate before final, so it may have to wait for 7.0.1.

Do you have an example of a c++ std library where this happens?

I'm using libc++ with the patch given above. I don't know about other libraries where iterators are pointers, only that this is allowed by the C++ standard.

we're currently not planning another release candidate before final, so it may have to wait for 7.0.1.

That's probably OK if all popular C++ libraries have iterator::value_type, although I'd be surprised if that's the case because its existence seems to be a non standardized convention.

Do you have an example of a c++ std library where this happens?

I have noticed that EASTL also has a pointer iterator: https://github.com/electronicarts/EASTL/blob/3.12.04/include/EASTL/vector.h#L20