This is an archive of the discontinued LLVM Phabricator instance.

Reland: [Demangle] replace use of llvm::StringView w/ std::string_view
ClosedPublic

Authored by nickdesaulniers on Apr 17 2023, 10:50 AM.

Details

Summary

This reverts commit d81cdb49d74064e88843733e7da92db865943509.

This refactoring was waiting on converting LLVM to C++17.

Leave StringView.h and cleanup around for subsequent cleanup.

Additional fixes for missing std::string_view conversions for MSVC.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 10:50 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
nickdesaulniers requested review of this revision.Apr 17 2023, 10:50 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 17 2023, 10:50 AM
nickdesaulniers planned changes to this revision.Apr 17 2023, 10:53 AM
nickdesaulniers added inline comments.
llvm/include/llvm/Demangle/ItaniumDemangle.h
19

I'll probably revert https://reviews.llvm.org/D148367 for now then rebase this on top.

I am confused why startswith is in the itanium demangle namespace but I could be confusing a specialised function with the generic one that works for any string view. Otherwise looks fine at a glance.

llvm/include/llvm/Demangle/ItaniumDemangle.h
1586

Is this right? Not sure itanium_demangle makes sense here.

2486

Same here, namespace doesn't sound right.

llvm/include/llvm/Demangle/MicrosoftDemangle.h
14

Needed because llvm's stringview included assert?

llvm/include/llvm/Demangle/Utility.h
19–20

Why does this change?

nickdesaulniers marked 2 inline comments as done.Apr 18 2023, 10:02 AM

I am confused why startswith is in the itanium demangle namespace but I could be confusing a specialised function with the generic one that works for any string view. Otherwise looks fine at a glance.

Hey David! Thanks for the review! See the two parent commits for more context:

  1. https://reviews.llvm.org/D148547
  2. https://reviews.llvm.org/D148556

The second in particular could use review still ;) The first has landed. This patch is part of a stack that could use review if you have the cycles for it.

llvm/include/llvm/Demangle/MicrosoftDemangle.h
14

yep, used below on L49.

llvm/include/llvm/Demangle/Utility.h
19–20

The definition of DEMANGLE_NAMESPACE_BEGIN used below on L30 comes from this header, which was being included indirectly by StringView.h before this change.

DavidSpickett accepted this revision.Apr 19 2023, 2:50 AM

I don't know enough to review the other patches but I understand the layering issue. So assuming the other patches are ok, this one LGTM.

llvm/include/llvm/Demangle/ItaniumDemangle.h
1586

Answered.

2486

Answered.

This revision is now accepted and ready to land.Apr 19 2023, 2:50 AM
MaskRay accepted this revision.Apr 19 2023, 1:11 PM
ayzhao requested changes to this revision.Apr 20 2023, 10:34 AM

This still fails to build on Windows with MSVC's cl.exe compiler:

C:\src\llvm-project\build-ninja>ninja all
[49/4908] Building CXX object lib\Demangle\CMakeFiles\LLVMDemangle.dir\ItaniumDemangle.cpp.obj
FAILED: lib/Demangle/CMakeFiles/LLVMDemangle.dir/ItaniumDemangle.cpp.obj
C:\PROGRA~1\MICROS~2\2022\PROFES~1\VC\Tools\MSVC\1435~1.322\bin\Hostx86\x86\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_FILE_OFFSET_BITS=64 -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_LARGEFILE_SOURCE -D_LIBCPP_ENABLE_ASSERTIONS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:\src\llvm-project\build-ninja\lib\Demangle -IC:\src\llvm-project\llvm\lib\Demangle -IC:\src\llvm-project\build-ninja\include -IC:\src\llvm-project\llvm\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2  /EHs-c- /GR- -UNDEBUG -std:c++17 /showIncludes /Folib\Demangle\CMakeFiles\LLVMDemangle.dir\ItaniumDemangle.cpp.obj /Fdlib\Demangle\CMakeFiles\LLVMDemangle.dir\LLVMDemangle.pdb /FS -c C:\src\llvm-project\llvm\lib\Demangle\ItaniumDemangle.cpp
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(3718): error C2665: 'llvm::itanium_demangle::ScopedOverride<const char *>::ScopedOverride': no overloaded function could convert all the argument types
C:\src\llvm-project\llvm\include\llvm\Demangle\Utility.h(194): note: could be 'llvm::itanium_demangle::ScopedOverride<const char *>::ScopedOverride(T &,T)'
        with
        [
            T=const char *
        ]
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(3718): note: 'llvm::itanium_demangle::ScopedOverride<const char *>::ScopedOverride(T &,T)': cannot convert argument 2 from 'std::_String_view_iterator<_Traits>' to 'T'
        with
        [
            T=const char *
        ]
        and
        [
            _Traits=std::char_traits<char>
        ]
        and
        [
            T=const char *
        ]
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(3718): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(3718): note: while trying to match the argument list '(const char *, std::_String_view_iterator<_Traits>)'
        with
        [
            _Traits=std::char_traits<char>
        ]
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(3707): note: while compiling class template member function 'llvm::itanium_demangle::Node *llvm::itanium_demangle::AbstractManglingParser<llvm::itanium_demangle::ManglingParser<`anonymous-namespace'::DefaultAllocator>,Alloc>::parseQualifiedType(void)'
        with
        [
            Alloc=`anonymous-namespace'::DefaultAllocator
        ]
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(3796): note: see reference to function template instantiation 'llvm::itanium_demangle::Node *llvm::itanium_demangle::AbstractManglingParser<llvm::itanium_demangle::ManglingParser<`anonymous-namespace'::DefaultAllocator>,Alloc>::parseQualifiedType(void)' being compiled
        with
        [
            Alloc=`anonymous-namespace'::DefaultAllocator
        ]
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(3472): note: while compiling class template member function 'std::string_view llvm::itanium_demangle::AbstractManglingParser<llvm::itanium_demangle::ManglingParser<`anonymous-namespace'::DefaultAllocator>,Alloc>::parseNumber(bool)'
        with
        [
            Alloc=`anonymous-namespace'::DefaultAllocator
        ]
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(5489): note: see reference to function template instantiation 'std::string_view llvm::itanium_demangle::AbstractManglingParser<llvm::itanium_demangle::ManglingParser<`anonymous-namespace'::DefaultAllocator>,Alloc>::parseNumber(bool)' being compiled
        with
        [
            Alloc=`anonymous-namespace'::DefaultAllocator
        ]
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(5469): note: while compiling class template member function 'llvm::itanium_demangle::Node *llvm::itanium_demangle::AbstractManglingParser<llvm::itanium_demangle::ManglingParser<`anonymous-namespace'::DefaultAllocator>,Alloc>::parse(void)'
        with
        [
            Alloc=`anonymous-namespace'::DefaultAllocator
        ]
C:\src\llvm-project\llvm\lib\Demangle\ItaniumDemangle.cpp(378): note: see reference to function template instantiation 'llvm::itanium_demangle::Node *llvm::itanium_demangle::AbstractManglingParser<llvm::itanium_demangle::ManglingParser<`anonymous-namespace'::DefaultAllocator>,Alloc>::parse(void)' being compiled
        with
        [
            Alloc=`anonymous-namespace'::DefaultAllocator
        ]
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(5505): note: see reference to class template instantiation 'llvm::itanium_demangle::AbstractManglingParser<llvm::itanium_demangle::ManglingParser<`anonymous-namespace'::DefaultAllocator>,Alloc>' being compiled
        with
        [
            Alloc=`anonymous-namespace'::DefaultAllocator
        ]
C:\src\llvm-project\llvm\lib\Demangle\ItaniumDemangle.cpp(377): note: see reference to class template instantiation 'llvm::itanium_demangle::ManglingParser<`anonymous-namespace'::DefaultAllocator>' being compiled
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(3719): error C2665: 'llvm::itanium_demangle::ScopedOverride<const char *>::ScopedOverride': no overloaded function could convert all the argument types
C:\src\llvm-project\llvm\include\llvm\Demangle\Utility.h(194): note: could be 'llvm::itanium_demangle::ScopedOverride<const char *>::ScopedOverride(T &,T)'
        with
        [
            T=const char *
        ]
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(3719): note: 'llvm::itanium_demangle::ScopedOverride<const char *>::ScopedOverride(T &,T)': cannot convert argument 2 from 'std::_String_view_iterator<_Traits>' to 'T'
        with
        [
            T=const char *
        ]
        and
        [
            _Traits=std::char_traits<char>
        ]
        and
        [
            T=const char *
        ]
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(3719): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
C:\src\llvm-project\llvm\include\llvm/Demangle/ItaniumDemangle.h(3719): note: while trying to match the argument list '(const char *, std::_String_view_iterator<_Traits>)'
        with
        [
            _Traits=std::char_traits<char>
        ]
[97/4908] Building CXX object tools\clang\utils\TableGen\CMakeFiles\clang-tblgen.dir\ClangAttrEmitter.cpp.obj
ninja: build stopped: subcommand failed.
This revision now requires changes to proceed.Apr 20 2023, 10:34 AM
nickdesaulniers marked 3 inline comments as done.
  • git clang-format HEAD~, fix 2 more conversion issues

A new conversion issue popped up:

C:\src\llvm-project\build-ninja>ninja all
[376/3206] Building CXX object lib\ProfileData\CMakeFiles\LLVMProfileData.dir\ItaniumManglingCanonicalizer.cpp.obj
FAILED: lib/ProfileData/CMakeFiles/LLVMProfileData.dir/ItaniumManglingCanonicalizer.cpp.obj
C:\PROGRA~1\MICROS~2\2022\PROFES~1\VC\Tools\MSVC\1435~1.322\bin\Hostx86\x86\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_FILE_OFFSET_BITS=64 -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_LARGEFILE_SOURCE -D_LIBCPP_ENABLE_ASSERTIONS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:\src\llvm-project\build-ninja\lib\ProfileData -IC:\src\llvm-project\llvm\lib\ProfileData -IC:\src\llvm-project\build-ninja\include -IC:\src\llvm-project\llvm\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2  /EHs-c- /GR- -UNDEBUG -std:c++17 /showIncludes /Folib\ProfileData\CMakeFiles\LLVMProfileData.dir\ItaniumManglingCanonicalizer.cpp.obj /Fdlib\ProfileData\CMakeFiles\LLVMProfileData.dir\LLVMProfileData.pdb /FS -c C:\src\llvm-project\llvm\lib\ProfileData\ItaniumManglingCanonicalizer.cpp
C:\src\llvm-project\llvm\lib\ProfileData\ItaniumManglingCanonicalizer.cpp(26): error C2440: '<function-style-cast>': cannot convert from 'initializer list' to 'llvm::StringRef'
C:\src\llvm-project\llvm\lib\ProfileData\ItaniumManglingCanonicalizer.cpp(26): note: No constructor could take the source type, or constructor overload resolution was ambiguous
[490/3022] Building CXX object tools\clang\lib\CodeGen\CMakeFiles\obj.clangCodeGen.dir\CGExpr.cpp.obj
C:\src\llvm-project\clang\lib\CodeGen\CGExpr.cpp(3583): warning C4018: '<=': signed/unsigned mismatch
[505/2927] Building CXX object tools\clang\lib\ASTMatchers\Dynamic\CMakeFiles\obj.clangDynamicASTMatchers.dir\Registry.cpp.obj
ninja: build stopped: subcommand failed.
  • fix another conversion issue for msvc
ayzhao accepted this revision.Apr 20 2023, 11:04 AM

LGTM, looks like this builds on Windows now:

C:\src\llvm-project\build-ninja>ninja all
[3498/6597] Building CXX object tools\clang\lib\AST\CMakeFiles\obj.clangAST.dir\AttrDocTable.cpp.obj
C:\src\llvm-project\clang\lib\AST\AttrDocTable.cpp(24): warning C4018: '<': signed/unsigned mismatch
[3846/6597] Building CXX object tools\clang\lib\CodeGen\CMakeFiles\obj.clangCodeGen.dir\CGExpr.cpp.obj
C:\src\llvm-project\clang\lib\CodeGen\CGExpr.cpp(3583): warning C4018: '<=': signed/unsigned mismatch
[4655/6597] Building CXX object tools\lld\COFF\CMakeFiles\lldCOFF.dir\PDB.cpp.obj
C:\src\llvm-project\lld\COFF\PDB.cpp(839): warning C4018: '>=': signed/unsigned mismatch
[4919/6597] Building CXX object tools\lldb\source\Core\CMakeFiles\lldbCore.dir\ModuleList.cpp.obj
C:\src\llvm-project\lldb\source\Core\ModuleList.cpp(336): warning C4996: 'std::shared_ptr<lldb_private::Module>::unique': warning STL4016: std::shared_ptr::unique() is deprecated in C++17. You can define _SILENCE_CXX17_SHARED_PTR_UNIQUE_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to suppress this warning.
C:\src\llvm-project\lldb\source\Core\ModuleList.cpp(367): warning C4996: 'std::shared_ptr<lldb_private::Module>::unique': warning STL4016: std::shared_ptr::unique() is deprecated in C++17. You can define _SILENCE_CXX17_SHARED_PTR_UNIQUE_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to suppress this warning.
[5175/6597] Building CXX object tools\lldb\source\Plugins\Process\Utility\CMakeFiles\lldbPluginProcessUtility.dir\NativeRegisterContextDBReg_x86.cpp.obj
C:\src\llvm-project\lldb\source\Plugins\Process\Utility\NativeRegisterContextDBReg_x86.h(23): warning C4589: Constructor of abstract class 'lldb_private::NativeRegisterContextDBReg_x86' ignores initializer for virtual base class 'lldb_private::NativeRegisterContextRegisterInfo'
C:\src\llvm-project\lldb\source\Plugins\Process\Utility\NativeRegisterContextDBReg_x86.h(23): note: virtual base classes are only initialized by the most-derived type
[5205/6597] Building CXX object tools\lldb\source\Plugins\LanguageRuntime\ObjC\AppleObjCRuntime\CMakeFiles\lldbPluginAppleObjCRuntime.dir\AppleObjCRuntimeV2.cpp.obj
C:\src\llvm-project\lldb\source\Plugins\LanguageRuntime\ObjC\AppleObjCRuntime\AppleObjCRuntimeV2.cpp(1744): warning C5030: attribute 'clang::fallthrough' is not recognized
[5263/6597] Building CXX object tools\lldb\source\Plugins\Process\Utility\CMakeFiles\lldbPluginProcessUtility.dir\RegisterContextPOSIX_mips64.cpp.obj
C:\src\llvm-project\lldb\source\Plugins\Process\Utility\RegisterContextPOSIX_mips64.cpp(109): warning C4065: switch statement contains 'default' but no 'case' labels
C:\src\llvm-project\lldb\source\Plugins\Process\Utility\RegisterContextPOSIX_mips64.cpp(120): warning C4065: switch statement contains 'default' but no 'case' labels
[5593/6597] Building CXX object tools\lldb\source\API\CMakeFiles\liblldb.dir\SBTypeFilter.cpp.obj
C:\src\llvm-project\lldb\source\API\SBTypeFilter.cpp(169): warning C4996: 'std::shared_ptr<lldb_private::TypeFilterImpl>::unique': warning STL4016: std::shared_ptr::unique() is deprecated in C++17. You can define _SILENCE_CXX17_SHARED_PTR_UNIQUE_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to suppress this warning.
[5652/6597] Building CXX object tools\lldb\source\API\CMakeFiles\liblldb.dir\SBTypeFormat.cpp.obj
C:\src\llvm-project\lldb\source\API\SBTypeFormat.cpp(160): warning C4996: 'std::shared_ptr<lldb_private::TypeFormatImpl>::unique': warning STL4016: std::shared_ptr::unique() is deprecated in C++17. You can define _SILENCE_CXX17_SHARED_PTR_UNIQUE_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to suppress this warning.
[5665/6597] Building CXX object tools\lldb\source\API\CMakeFiles\liblldb.dir\SBTypeSynthetic.cpp.obj
C:\src\llvm-project\lldb\source\API\SBTypeSynthetic.cpp(187): warning C4996: 'std::shared_ptr<lldb_private::ScriptedSyntheticChildren>::unique': warning STL4016: std::shared_ptr::unique() is deprecated in C++17. You can define _SILENCE_CXX17_SHARED_PTR_UNIQUE_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to suppress this warning.
[5675/6597] Building CXX object tools\lldb\source\API\CMakeFiles\liblldb.dir\SBTypeSummary.cpp.obj
C:\src\llvm-project\lldb\source\API\SBTypeSummary.cpp(384): warning C4996: 'std::shared_ptr<lldb_private::TypeSummaryImpl>::unique': warning STL4016: std::shared_ptr::unique() is deprecated in C++17. You can define _SILENCE_CXX17_SHARED_PTR_UNIQUE_DEPRECATION_WARNING or _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS to suppress this warning.
[6585/6597] Building CXX object tools\clang\tools\extra\clangd\CMakeFiles\obj.clangDaemon.dir\InlayHints.cpp.obj
C:\src\llvm-project\clang-tools-extra\clangd\InlayHints.cpp(594): warning C4018: '<': signed/unsigned mismatch
[6597/6597] Linking CXX executable bin\clangd.exe
This revision is now accepted and ready to land.Apr 20 2023, 11:04 AM

Thanks for the help testing on Windows @ayzhao . Those warnings look potentially interesting to follow up; none appear to be introduced from this diff. Landing.

This revision was landed with ongoing or failed builds.Apr 20 2023, 11:22 AM
This revision was automatically updated to reflect the committed changes.

llvm/test/Demangle/ms-cxx14.test is now crashing on windows, working on a fix forward for llvm::microsoftDemangle which I think is missing a nullptr check.