Remove StringLiteral in favor of StringRef

[bjacob]

StringLiteral was added 9 years ago in https://reviews.llvm.org/D27686. It is a subclass of StringRef with a constructor taking a string literal: template <size_t N> constexpr StringLiteral(const char (&Str)[N]) .

I don't know about 9 years ago, but I would guess that on current compilers, strlen is a builtin, and passing a string literal to an inline StringRef constructor that calls strlen will have the same outcome as calling the above templated StringLiteral constructor.

Here is a Compiler Explorer experiment to tell if this still makes any difference on current compilers:
https://godbolt.org/z/Y8rxY99Mv
Result:

• It doesn't make a difference, the strlen is constant-evaluated on both Clang and GCC as far back as Compiler Explorer supports (Clang 9 and GCC 4.5).
• It does make a difference, the strlen is NOT constant-evaluated on MSVC, even the latest version.

Do we care about performance on MSVC ?

EDIT: let's just add a StringLiteral-like templated constructor taking a char array, to StringRef. Won't make any difference on Clang and GCC but should help MSVC generate the same code.

EDIT2: see comments below, I was confused, MSVC is optimizing the strlen.

@joker-eph (reviewer of the original https://reviews.llvm.org/D27686).

Side note: my motivation in removing StringLiteral is that it is a bit of a footgun. While the original https://reviews.llvm.org/D27686 clearly states that it's intended for arrays of string literals, where it would not be possible anyway to store each literal as a true literal, usage has crept to many cases where StringLiteral is used instead of const char[] where the latter would be possible, for individual string literals. That's a problem as evaluating the address of a string literal uses a relocation. See iree-org/iree#19529. So if we want to keep StringLiteral we need to be clear exactly what it's good and not good for. It is good to work around MSVC not constant-evaluating strlen, and it is only to be used where we can't use a true const char[] literal, such as arrays of string literals.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 0aa831e0edb1c1deabb96ce2435667cc82bac79b 8d50311b9a40662d6d430bc22af809b85201b7ac --extensions c,cpp,h -- clang-tools-extra/clang-query/QueryParser.cpp clang-tools-extra/clang-tidy/ClangTidyCheck.cpp clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp clang-tools-extra/clang-tidy/abseil/AbseilMatcher.h clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp clang-tools-extra/clang-tidy/android/CloexecCheck.cpp clang-tools-extra/clang-tidy/android/CloexecFopenCheck.h clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp clang-tools-extra/clang-tidy/bugprone/StringLiteralWithEmbeddedNulCheck.cpp clang-tools-extra/clang-tidy/bugprone/SuspiciousMissingCommaCheck.cpp clang-tools-extra/clang-tidy/bugprone/TaggedUnionMemberCountCheck.cpp clang-tools-extra/clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp clang-tools-extra/clang-tidy/cert/StrToNumCheck.cpp clang-tools-extra/clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.cpp clang-tools-extra/clang-tidy/modernize/RawStringLiteralCheck.h clang-tools-extra/clang-tidy/modernize/UnaryStaticAssertCheck.cpp clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp clang-tools-extra/clang-tidy/modernize/UseStartsEndsWithCheck.cpp clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp clang-tools-extra/clang-tidy/objc/NSDateFormatterCheck.cpp clang-tools-extra/clang-tidy/performance/FasterStringFindCheck.cpp clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.cpp clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp clang-tools-extra/clang-tidy/readability/MisplacedArrayIndexCheck.cpp clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp clang-tools-extra/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp clang-tools-extra/clang-tidy/utils/FormatStringConverter.h clang-tools-extra/clangd/ClangdLSPServer.cpp clang-tools-extra/clangd/ClangdServer.h clang-tools-extra/clangd/ConfigCompile.cpp clang-tools-extra/clangd/ConfigYAML.cpp clang-tools-extra/clangd/Hover.cpp clang-tools-extra/clangd/InlayHints.cpp clang-tools-extra/clangd/LSPBinder.h clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/Preamble.h clang-tools-extra/clangd/Protocol.cpp clang-tools-extra/clangd/Protocol.h clang-tools-extra/clangd/Selection.cpp clang-tools-extra/clangd/SemanticSelection.cpp clang-tools-extra/clangd/TUScheduler.cpp clang-tools-extra/clangd/TidyProvider.cpp clang-tools-extra/clangd/index/StdLib.cpp clang-tools-extra/clangd/index/remote/server/Server.cpp clang-tools-extra/clangd/refactor/Tweak.h clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp clang-tools-extra/clangd/refactor/tweaks/DumpAST.cpp clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp clang-tools-extra/clangd/refactor/tweaks/ObjCLocalizeStringLiteral.cpp clang-tools-extra/clangd/refactor/tweaks/ObjCMemberwiseInitializer.cpp clang-tools-extra/clangd/refactor/tweaks/PopulateSwitch.cpp clang-tools-extra/clangd/refactor/tweaks/RawStringLiteral.cpp clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp clang-tools-extra/clangd/refactor/tweaks/ScopifyEnum.cpp clang-tools-extra/clangd/refactor/tweaks/SpecialMembers.cpp clang-tools-extra/clangd/refactor/tweaks/SwapBinaryOperands.cpp clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp clang-tools-extra/clangd/support/MemoryTree.h clang-tools-extra/clangd/support/Trace.h clang-tools-extra/clangd/unittests/ASTTests.cpp clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp clang-tools-extra/clangd/unittests/ClangdTests.cpp clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp clang-tools-extra/clangd/unittests/InsertionPointTests.cpp clang-tools-extra/clangd/unittests/ParsedASTTests.cpp clang-tools-extra/clangd/unittests/PreambleTests.cpp clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp clang-tools-extra/clangd/unittests/ReplayPeambleTests.cpp clang-tools-extra/clangd/unittests/SelectionTests.cpp clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp clang-tools-extra/clangd/unittests/TestTU.cpp clang-tools-extra/clangd/unittests/XRefsTests.cpp clang-tools-extra/clangd/unittests/support/MemoryTreeTests.cpp clang-tools-extra/clangd/unittests/support/TraceTests.cpp clang-tools-extra/include-cleaner/lib/HTMLReport.cpp clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp clang-tools-extra/include-cleaner/unittests/RecordTest.cpp clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp clang/examples/Attribute/Attribute.cpp clang/include/clang/AST/ASTContext.h clang/include/clang/AST/Decl.h clang/include/clang/AST/Expr.h clang/include/clang/AST/ExprObjC.h clang/include/clang/AST/JSONNodeDumper.h clang/include/clang/AST/Mangle.h clang/include/clang/AST/RecursiveASTVisitor.h clang/include/clang/AST/Stmt.h clang/include/clang/AST/TextNodeDumper.h clang/include/clang/ASTMatchers/ASTMatchers.h clang/include/clang/ASTMatchers/ASTMatchersInternal.h clang/include/clang/ASTMatchers/Dynamic/Parser.h clang/include/clang/Basic/Builtins.h clang/include/clang/ExtractAPI/DeclarationFragments.h clang/include/clang/Parse/Parser.h clang/include/clang/Sema/Sema.h clang/include/clang/Sema/SemaObjC.h clang/include/clang/Serialization/ASTBitCodes.h clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h clang/include/clang/Tooling/Syntax/TokenBufferTokenManager.h clang/include/clang/Tooling/Syntax/TokenManager.h clang/lib/AST/ASTContext.cpp clang/lib/AST/ASTImporter.cpp clang/lib/AST/ASTStructuralEquivalence.cpp clang/lib/AST/ByteCode/Compiler.cpp clang/lib/AST/ByteCode/Compiler.h clang/lib/AST/ByteCode/InterpBuiltin.cpp clang/lib/AST/ByteCode/Pointer.cpp clang/lib/AST/ByteCode/Program.cpp clang/lib/AST/ByteCode/Program.h clang/lib/AST/Decl.cpp clang/lib/AST/DeclPrinter.cpp clang/lib/AST/Expr.cpp clang/lib/AST/ExprClassification.cpp clang/lib/AST/ExprConstant.cpp clang/lib/AST/ItaniumMangle.cpp clang/lib/AST/JSONNodeDumper.cpp clang/lib/AST/MicrosoftMangle.cpp clang/lib/AST/ODRHash.cpp clang/lib/AST/OSLog.cpp clang/lib/AST/OpenMPClause.cpp clang/lib/AST/Stmt.cpp clang/lib/AST/StmtPrinter.cpp clang/lib/AST/StmtProfile.cpp clang/lib/AST/TextNodeDumper.cpp clang/lib/ASTMatchers/ASTMatchersInternal.cpp clang/lib/Analysis/CalledOnceCheck.cpp clang/lib/Analysis/CloneDetection.cpp clang/lib/Analysis/FlowSensitive/Formula.cpp clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp clang/lib/Analysis/ReachableCode.cpp clang/lib/Analysis/ThreadSafetyCommon.cpp clang/lib/Analysis/UnsafeBufferUsage.cpp clang/lib/Basic/SourceManager.cpp clang/lib/Basic/Targets/BPF.cpp clang/lib/Basic/Targets/Hexagon.cpp clang/lib/Basic/Targets/Mips.cpp clang/lib/Basic/Targets/Sparc.cpp clang/lib/Basic/Targets/SystemZ.cpp clang/lib/Basic/Targets/WebAssembly.cpp clang/lib/CodeGen/CGBuiltin.cpp clang/lib/CodeGen/CGCoroutine.cpp clang/lib/CodeGen/CGDebugInfo.cpp clang/lib/CodeGen/CGDebugInfo.h clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/CGExprAgg.cpp clang/lib/CodeGen/CGExprCXX.cpp clang/lib/CodeGen/CGExprConstant.cpp clang/lib/CodeGen/CGObjCGNU.cpp clang/lib/CodeGen/CGObjCMac.cpp clang/lib/CodeGen/CGObjCRuntime.h clang/lib/CodeGen/CGOpenMPRuntime.cpp clang/lib/CodeGen/CGStmt.cpp clang/lib/CodeGen/CodeGenFunction.h clang/lib/CodeGen/CodeGenModule.cpp clang/lib/CodeGen/CodeGenModule.h clang/lib/Driver/ToolChains/BareMetal.cpp clang/lib/Driver/ToolChains/Clang.cpp clang/lib/Driver/ToolChains/CommonArgs.cpp clang/lib/Driver/ToolChains/Darwin.cpp clang/lib/Edit/RewriteObjCFoundationAPI.cpp clang/lib/ExtractAPI/DeclarationFragments.cpp clang/lib/Format/Format.cpp clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp clang/lib/Frontend/Rewrite/RewriteObjC.cpp clang/lib/Lex/ModuleMap.cpp clang/lib/Parse/ParseCXXInlineMethods.cpp clang/lib/Parse/ParseDeclCXX.cpp clang/lib/Parse/ParseInit.cpp clang/lib/Parse/ParseOpenMP.cpp clang/lib/Parse/ParsePragma.cpp clang/lib/Parse/ParseStmtAsm.cpp clang/lib/Parse/Parser.cpp clang/lib/Sema/SemaARM.cpp clang/lib/Sema/SemaAttr.cpp clang/lib/Sema/SemaCast.cpp clang/lib/Sema/SemaChecking.cpp clang/lib/Sema/SemaCodeComplete.cpp clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaDeclAttr.cpp clang/lib/Sema/SemaDeclCXX.cpp clang/lib/Sema/SemaExpr.cpp clang/lib/Sema/SemaExprCXX.cpp clang/lib/Sema/SemaExprObjC.cpp clang/lib/Sema/SemaInit.cpp clang/lib/Sema/SemaLookup.cpp clang/lib/Sema/SemaObjC.cpp clang/lib/Sema/SemaOpenMP.cpp clang/lib/Sema/SemaOverload.cpp clang/lib/Sema/SemaStmtAsm.cpp clang/lib/Sema/SemaTemplateDeduction.cpp clang/lib/Sema/SemaType.cpp clang/lib/Sema/TreeTransform.h clang/lib/Serialization/ASTReaderDecl.cpp clang/lib/Serialization/ASTReaderStmt.cpp clang/lib/Serialization/ASTWriterDecl.cpp clang/lib/Serialization/ASTWriterStmt.cpp clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp clang/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp clang/lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp clang/lib/StaticAnalyzer/Checkers/SetgidSetuidOrderChecker.cpp clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp clang/lib/StaticAnalyzer/Checkers/WebKit/MemoryUnsafeCastChecker.cpp clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp clang/lib/StaticAnalyzer/Core/BugReporter.cpp clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp clang/lib/StaticAnalyzer/Core/MemRegion.cpp clang/lib/StaticAnalyzer/Core/RegionStore.cpp clang/lib/StaticAnalyzer/Core/SValBuilder.cpp clang/lib/Tooling/ASTDiff/ASTDiff.cpp clang/lib/Tooling/Inclusions/HeaderAnalysis.cpp clang/lib/Tooling/Refactoring/ASTSelection.cpp clang/lib/Tooling/Syntax/BuildTree.cpp clang/lib/Tooling/Syntax/TokenBufferTokenManager.cpp clang/test/AST/ByteCode/records.cpp clang/test/AST/ast-dump-cxx2c-delete-with-message.cpp clang/test/AST/ast-dump-decl.cpp clang/test/AST/ast-dump-expr-json.c clang/test/AST/ast-dump-expr.c clang/test/AST/ast-dump-recovery.c clang/test/AST/ast-dump-recovery.cpp clang/test/AST/ast-dump-wchar.cpp clang/test/AST/explicit-base-class-move-cntr.cpp clang/test/Analysis/array-struct.c clang/test/C/C23/n2322.c clang/test/CXX/drs/cwg177x.cpp clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp clang/test/Frontend/plugin-attribute.cpp clang/test/Index/annotate-tokens.c clang/test/Index/load-staticassert.cpp clang/test/Index/recursive-cxx-member-calls.cpp clang/test/OpenMP/interop_ast_print.cpp clang/test/PCH/exprs.c clang/test/PCH/exprs.h clang/test/SemaCXX/constant-expression-cxx11.cpp clang/test/SemaCXX/lambda-expressions.cpp clang/test/SemaTemplate/instantiate-expr-basic.cpp clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp clang/test/Tooling/clang-diff-ast.cpp clang/test/Tooling/clang-diff-basic.cpp clang/tools/libclang/CIndex.cpp clang/unittests/AST/ASTImporterTest.cpp clang/unittests/AST/ASTTraverserTest.cpp clang/unittests/AST/AttrTest.cpp clang/unittests/AST/DeclPrinterTest.cpp clang/unittests/AST/StmtPrinterTest.cpp clang/unittests/AST/StructuralEquivalenceTest.cpp clang/unittests/AST/TypePrinterTest.cpp clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp clang/unittests/ASTMatchers/GtestMatchersTest.cpp clang/unittests/Basic/DiagnosticTest.cpp clang/unittests/Basic/SourceManagerTest.cpp clang/unittests/Format/FormatTestVerilog.cpp clang/unittests/Lex/LexerTest.cpp clang/unittests/Tooling/CastExprTest.cpp clang/unittests/Tooling/RecursiveASTVisitorTests/MemberPointerTypeLoc.cpp clang/unittests/Tooling/Syntax/BuildTreeTest.cpp clang/unittests/Tooling/Syntax/TokensTest.cpp clang/utils/TableGen/ClangDiagnosticsEmitter.cpp clang/utils/TableGen/ClangOptionDocEmitter.cpp flang/include/flang/Optimizer/Dialect/FIRAttr.h flang/lib/Optimizer/Builder/IntrinsicCall.cpp flang/lib/Parser/source.cpp flang/unittests/Frontend/CodeGenActionTest.cpp libcxxabi/src/demangle/ItaniumDemangle.h libcxxabi/test/test_demangle.pass.cpp lld/MachO/SyntheticSections.h lldb/include/lldb/Interpreter/CommandInterpreter.h lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h lldb/include/lldb/Interpreter/Options.h lldb/include/lldb/Target/UnixSignals.h lldb/include/lldb/Utility/Log.h lldb/source/Core/Debugger.cpp lldb/source/Core/Disassembler.cpp lldb/source/Core/PluginManager.cpp lldb/source/Core/ThreadedCommunication.cpp lldb/source/Core/UserSettingsController.cpp lldb/source/Expression/REPL.cpp lldb/source/Host/common/ZipFileResolver.cpp lldb/source/Host/windows/PipeWindows.cpp lldb/source/Interpreter/CommandInterpreter.cpp lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwinProperties.cpp lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp lldb/source/Plugins/Language/CPlusPlus/CPlusPlusNameParser.cpp lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp lldb/source/Plugins/Language/ObjC/CF.cpp lldb/source/Plugins/Language/ObjC/Cocoa.cpp lldb/source/Plugins/Language/ObjC/NSArray.cpp lldb/source/Plugins/Language/ObjC/NSDictionary.cpp lldb/source/Plugins/Language/ObjC/NSSet.cpp lldb/source/Plugins/Language/ObjC/NSString.cpp lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Plugins/REPL/Clang/ClangREPL.cpp lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp lldb/source/Plugins/SymbolFile/DWARF/DWARFDefines.cpp lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp lldb/source/Plugins/SymbolFile/DWARF/NameToDIE.cpp lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.cpp lldb/source/Symbol/Symtab.cpp lldb/source/Target/Language.cpp lldb/source/Target/Platform.cpp lldb/source/Target/Process.cpp lldb/source/Target/Target.cpp lldb/source/Target/TargetList.cpp lldb/source/Target/Thread.cpp lldb/source/Target/UnixSignals.cpp lldb/source/Utility/Broadcaster.cpp lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp lldb/unittests/Target/LocateModuleCallbackTest.cpp lldb/unittests/Utility/ListenerTest.cpp llvm/include/llvm/ADT/StringExtras.h llvm/include/llvm/ADT/StringRef.h llvm/include/llvm/ADT/StringSwitch.h llvm/include/llvm/ADT/StringTable.h llvm/include/llvm/ADT/Twine.h llvm/include/llvm/Analysis/TargetLibraryInfo.h llvm/include/llvm/CodeGen/TargetRegisterInfo.h llvm/include/llvm/DWARFLinker/DWARFLinkerBase.h llvm/include/llvm/DWARFLinker/Parallel/DWARFLinker.h llvm/include/llvm/Demangle/ItaniumDemangle.h llvm/include/llvm/Frontend/OpenMP/OMPAssume.h llvm/include/llvm/LTO/LTO.h llvm/include/llvm/Remarks/BitstreamRemarkContainer.h llvm/include/llvm/Remarks/RemarkFormat.h llvm/include/llvm/Support/JSON.h llvm/include/llvm/Support/RISCVISAUtils.h llvm/include/llvm/TargetParser/RISCVTargetParser.h llvm/include/llvm/Telemetry/Telemetry.h llvm/include/llvm/TextAPI/Symbol.h llvm/lib/Analysis/TargetLibraryInfo.cpp llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp llvm/lib/FileCheck/FileCheck.cpp llvm/lib/InterfaceStub/ELFObjHandler.cpp llvm/lib/LTO/LTO.cpp llvm/lib/MC/MCSectionMachO.cpp llvm/lib/Support/CommandLine.cpp llvm/lib/Support/FloatingPointMode.cpp llvm/lib/Support/FormatVariadic.cpp llvm/lib/Support/JSON.cpp llvm/lib/Support/YAMLTraits.cpp llvm/lib/Target/AArch64/AArch64MCInstLower.cpp llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp llvm/lib/Target/AMDGPU/SIISelLowering.cpp llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp llvm/lib/Target/AMDGPU/SIRegisterInfo.h llvm/lib/Target/AMDGPU/Utils/AMDGPUAsmUtils.cpp llvm/lib/Target/AMDGPU/Utils/AMDGPUAsmUtils.h llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp llvm/lib/Target/AMDGPU/Utils/AMDKernelCodeTUtils.cpp llvm/lib/Target/DirectX/DXILOpBuilder.cpp llvm/lib/Target/X86/MCTargetDesc/X86InstComments.cpp llvm/lib/Target/X86/X86InsertPrefetch.cpp llvm/lib/TargetParser/PPCTargetParser.cpp llvm/lib/TargetParser/RISCVISAInfo.cpp llvm/lib/TargetParser/TargetParser.cpp llvm/lib/TargetParser/X86TargetParser.cpp llvm/lib/TextAPI/Utils.cpp llvm/lib/Transforms/HipStdPar/HipStdPar.cpp llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp llvm/lib/WindowsManifest/WindowsManifestMerger.cpp llvm/tools/llvm-exegesis/lib/Analysis.cpp llvm/tools/llvm-exegesis/lib/LlvmState.h llvm/tools/llvm-readtapi/DiffEngine.cpp llvm/tools/llvm-readtapi/DiffEngine.h llvm/tools/llvm-reduce/deltas/ReduceMetadata.cpp llvm/tools/opt/optdriver.cpp llvm/unittests/ADT/ScopedHashTableTest.cpp llvm/unittests/ADT/StringRefTest.cpp llvm/unittests/ADT/StringSwitchTest.cpp llvm/unittests/ADT/TwineTest.cpp llvm/unittests/IR/ModuleTest.cpp llvm/unittests/Support/BinaryStreamTest.cpp llvm/unittests/Support/DJBTest.cpp llvm/unittests/Support/FormatVariadicTest.cpp llvm/unittests/Support/Path.cpp llvm/unittests/Telemetry/TelemetryTest.cpp llvm/utils/TableGen/Basic/CodeGenIntrinsics.cpp llvm/utils/TableGen/Basic/IntrinsicEmitter.cpp llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp llvm/utils/TableGen/Common/GlobalISel/PatternParser.cpp llvm/utils/TableGen/Common/GlobalISel/Patterns.h llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp mlir/examples/toy/Ch7/include/toy/Dialect.h mlir/include/mlir/Dialect/GPU/IR/CompilationInterfaces.h mlir/include/mlir/Dialect/GPU/IR/GPUDialect.h mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h mlir/include/mlir/Dialect/OpenACC/OpenACC.h mlir/include/mlir/Dialect/Quant/IR/QuantTypes.h mlir/include/mlir/Dialect/SPIRV/IR/SPIRVAttributes.h mlir/include/mlir/Dialect/SPIRV/IR/SPIRVTypes.h mlir/include/mlir/IR/Action.h mlir/include/mlir/IR/BuiltinAttributes.h mlir/include/mlir/IR/DialectImplementation.h mlir/include/mlir/IR/OpImplementation.h mlir/include/mlir/Pass/Pass.h mlir/include/mlir/Rewrite/PatternApplicator.h mlir/include/mlir/Support/LLVM.h mlir/include/mlir/Target/LLVMIR/ModuleImport.h mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h mlir/include/mlir/Tools/lsp-server-support/Protocol.h mlir/include/mlir/Tools/lsp-server-support/Transport.h mlir/lib/Bytecode/Writer/BytecodeWriter.cpp mlir/lib/Conversion/ArmSMEToLLVM/ArmSMEToLLVM.cpp mlir/lib/Conversion/GPUToLLVMSPV/GPUToLLVMSPV.cpp mlir/lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp mlir/lib/Conversion/SCFToGPU/SCFToGPU.cpp mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp mlir/lib/Dialect/Affine/IR/AffineOps.cpp mlir/lib/Dialect/ArmSME/Transforms/EnableArmStreaming.cpp mlir/lib/Dialect/ArmSME/Transforms/OuterProductFusion.cpp mlir/lib/Dialect/ArmSME/Transforms/VectorLegalization.cpp mlir/lib/Dialect/ArmSVE/Transforms/LegalizeVectorStorage.cpp mlir/lib/Dialect/Bufferization/IR/BufferizationDialect.cpp mlir/lib/Dialect/Bufferization/Transforms/OneShotAnalysis.cpp mlir/lib/Dialect/DLTI/DLTI.cpp mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp mlir/lib/Dialect/Linalg/IR/LinalgDialect.cpp mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp mlir/lib/Dialect/SparseTensor/Transforms/Utils/LoopEmitter.h mlir/lib/Dialect/Transform/Interfaces/MatchInterfaces.cpp mlir/lib/Pass/PassStatistics.cpp mlir/lib/Query/Matcher/Parser.h mlir/lib/Query/QueryParser.cpp mlir/lib/Support/Timing.cpp mlir/lib/Target/LLVMIR/DataLayoutImporter.cpp mlir/lib/Target/LLVMIR/DataLayoutImporter.h mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMIRToLLVMTranslation.cpp mlir/lib/Target/LLVMIR/ModuleImport.cpp mlir/lib/Target/LLVMIR/ModuleTranslation.cpp mlir/lib/Tools/lsp-server-support/Protocol.cpp mlir/lib/Tools/mlir-pdll-lsp-server/Protocol.cpp mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp mlir/lib/Transforms/Utils/WalkPatternRewriteDriver.cpp mlir/test/lib/Analysis/DataFlow/TestDenseBackwardDataFlowAnalysis.cpp mlir/test/lib/Analysis/TestSlice.cpp mlir/test/lib/Dialect/SCF/TestSCFUtils.cpp mlir/test/lib/Dialect/Test/TestTypes.h mlir/test/lib/Dialect/Transform/TestTransformDialectExtension.cpp mlir/test/lib/IR/TestBytecodeRoundtrip.cpp mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp mlir/tools/mlir-tblgen/DialectGen.cpp mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp mlir/tools/mlir-tblgen/PassGen.cpp mlir/tools/mlir-tblgen/SPIRVUtilsGen.cpp mlir/unittests/Bytecode/BytecodeTest.cpp mlir/unittests/Debug/DebugCounterTest.cpp mlir/unittests/Debug/ExecutionContextTest.cpp mlir/unittests/Debug/FileLineColLocBreakpointManagerTest.cpp mlir/unittests/Dialect/Transform/Preload.cpp mlir/unittests/IR/OpPropertiesTest.cpp mlir/unittests/IR/SymbolTableTest.cpp mlir/unittests/IR/TypeAttrNamesTest.cpp mlir/unittests/IR/TypeTest.cpp mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp mlir/unittests/Parser/ParserTest.cpp

View the diff from clang-format here.

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 67b8c6f460..7c461680e5 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -5828,7 +5828,7 @@ class FormatStringLiteral {
 
    unsigned getByteLength() const {
      return FExpr->getByteLength() - getCharByteWidth() * Offset;
-  }
+   }
 
   unsigned getLength() const { return FExpr->getLength() - Offset; }
   unsigned getCharByteWidth() const { return FExpr->getCharByteWidth(); }

[QuietMisdreavus]

This is an unnecessary find-and-replace. This enum constant specifically references a string literal, not the StringLiteral type.

[bjacob]

good catch! i'll pore over the actual diff line by line to catch more things like this, but I wanted to first see if this PR would be shot down at a higher level :-)

[nikic]

Generally a lot of clang changes are wrong. There's a StringLiteral AST node, without relation to StringRef...

[bjacob]

Thanks for the heads up -- will fix one by one.

Also, note to self: let's leep StringLiteral around as deprecated to give downstreams a smoother transition.

[kuhar]

Note that now we also have a new type for string tables: #119488 which doesn't result in relocations

[nikic]

I think StringLiteral is often used as an additional safety net, to ensure(*) that only string literals get passed and we don't end up with dangling pointers. This is especially useful when migrating some code from using an owned to an unowned string.

(*) It doesn't strictly ensure that, but good enough for all practical purposes.

[nikic]

I didn't really understand the last paragraph of the PR description. How does using StringRef instead of StringLiteral reduce relocations?

[jrtc27]

Side note: my motivation in removing StringLiteral is that it is a bit of a footgun. While the original https://reviews.llvm.org/D27686 clearly states that it's intended for arrays of string literals, where it would not be possible anyway to store each literal as a true literal, usage has crept to many cases where StringLiteral is used instead of const char[] where the latter would be possible, for individual string literals. That's a problem as evaluating the address of a string literal uses a relocation. See iree-org/iree#19529. So if we want to keep StringLiteral we need to be clear exactly what it's good and not good for. It is good to work around MSVC not constant-evaluating strlen, and it is only to be used where we can't use a true const char[] literal, such as arrays of string literals.

const char[] allocates storage for the string contents though, so takes up space in memory if the string can be merged with others at compile or link time. There are multiple things being traded off here, and PDE vs PIC/PIE complicates the story further. So it's not just as simple as "use const char[] whenever you can".

[bjacob]

@nikic

I didn't really understand the last paragraph of the PR description. How does using StringRef instead of StringLiteral reduce relocations?

It does not - sorry if that was confusingly worded. What I was trying to say is that using a plain const char[] instead of a StringLiteral avoids one relocation. The problem then with StringLiteral is that its existence and name naturally lead people to use it for things that could have been a const char[] , resulting in more relocations. Maybe not having a class named StringLiteral will result in more string literals remaining const char[].

[s-barannikov]

• the strlen is NOT constant-evaluated on MSVC, even the latest version.

The godbolt link shows otherwise?

[bjacob]

@jrtc27

const char[] allocates storage for the string contents though, so takes up space in memory if the string can be merged with others at compile or link time. There are multiple things being traded off here, and PDE vs PIC/PIE complicates the story further. So it's not just as simple as "use const char[] whenever you can".

Ah, thanks for the explanation... I really hadn't thought of that, but IIUC you're saying that the indirection (referring to a string via a pointer to it) allows merging storage when a string literal is a substring of another, while using raw const char[] forces no sharing.

Really interesting, but I still wonder -- this sounds like a fancier kind of optimization that probably won't be able to apply in a majority of cases (it needs literals to be substrings of each other?) while the relocations are every single time.

[bjacob]

@s-barannikov

• the strlen is NOT constant-evaluated on MSVC, even the latest version.

The godbolt link shows otherwise?

This is what I see in godbolt - to me this loop looks like strlen:

[s-barannikov]

@s-barannikov

• the strlen is NOT constant-evaluated on MSVC, even the latest version.

The godbolt link shows otherwise?

This is what I see in godbolt - to me this loop looks like strlen:

It is the body of the constructor. xyz function is below

StringRef xyz(void) PROC                     ; xyz, COMDAT
        lea     rax, OFFSET FLAT:`string'
        mov     DWORD PTR [rcx+8], 26
        mov     QWORD PTR [rcx], rax
        mov     rax, rcx
        ret     0
StringRef xyz(void) ENDP                     ; xyz

[jrtc27]

@jrtc27

const char[] allocates storage for the string contents though, so takes up space in memory if the string can be merged with others at compile or link time. There are multiple things being traded off here, and PDE vs PIC/PIE complicates the story further. So it's not just as simple as "use const char[] whenever you can".

Ah, thanks for the explanation... I really hadn't thought of that, but IIUC you're saying that the indirection (referring to a string via a pointer to it) allows merging storage when a string literal is a substring of another, while using raw const char[] doesn't allow any reuse unless the string literal exactly match.

No, const char[] can never merge because each object has a unique address and the string's contents are stored in the object itself.

Really interesting, but I still wonder -- this sounds like a fancier kind of optimization that probably won't be able to apply in a majority of cases (it needs literals to be substrings of each other?) while the relocations are every single time.

Yes, you need to have strings where one is a suffix of another. Some types of string literals get reused all over the place (e.g. some key that comes from a header) and so will benefit greatly from this; the diff you posted in iree-org/iree#19529 is an example of the kind of thing that does benefit from this merging, so you may well find that your memory footprint increases slightly in order to remove that run-time relocation cost. Others really don't (e.g. a long error message).

Note that, at least for ELF, this can be done at link time (search for SHF_MERGE), so it's a whole-program optimisation, and does not require LTO to be done like that.

[bjacob]

@nikic

I think StringLiteral is often used as an additional safety net, to ensure(*) that only string literals get passed and we don't end up with dangling pointers. This is especially useful when migrating some code from using an owned to an unowned string.

(*) It doesn't strictly ensure that, but good enough for all practical purposes.

Oh, I see. That sounds like a good high level reason to not go forward with this PR, then. I didn't know/think of that soft-contract / cultural aspect of StringLiteral.

I'll proactively close this PR now to avoid it becoming more time-consuming.

[ldionne]

FWIW if this ever gets revived, the changes in libcxxabi were unnecessary. In that part of the code we're not using llvm::StringLiteral, we're defining a class with the name StringLiteral to represent string literals in mangled names, and it makes sense to keep it that way.

[bjacob]

@jrtc27

Ah, thanks for the explanation... I really hadn't thought of that, but IIUC you're saying that the indirection (referring to a string via a pointer to it) allows merging storage when a string literal is a substring of another, while using raw const char[] doesn't allow any reuse unless the string literal exactly match.

No, const char[] can never merge because each object has a unique address and the string's contents are stored in the object itself.

Ha yes, I realized that and edited my own comment to that effect. I get it now!

Yes, you need to have strings where one is a suffix of another. Some types of string literals get reused all over the place (e.g. some key that comes from a header) and so will benefit greatly from this; the diff you posted in iree-org/iree#19529 is an example of the kind of thing that does benefit from this merging, so you may well find that your memory footprint increases slightly in order to remove that run-time relocation cost. Others really don't (e.g. a long error message).

Note that, at least for ELF, this can be done at link time (search for SHF_MERGE), so it's a whole-program optimisation, and does not require LTO to be done like that.

Thanks a lot for the explanation!

[bjacob]

It is the body of the constructor. xyz function is below

@s-barannikov , thanks... i was confused by the inline StringRef constructor being emitted by MSVC while it wasn't by Clang and GCC.

[efriedma-quic]

For reference, when it was added, the important thing about StringLiteral was that it had a constexpr constructor. The StringRef constructor became constexpr with 57effbd, a few years after StringLiteral was added.

[jh7370]

Do we care about performance on MSVC ?

Just responding to this point (although I accept it's moot in the current context): yes we care about performance when building with MSVC - our downstream toolchain is hosted on Windows. I'm sure we're not the only Windows client either.

[dwblaikie]

Do we care about performance on MSVC ?

Just responding to this point (although I accept it's moot in the current context): yes we care about performance when building with MSVC - our downstream toolchain is hosted on Windows. I'm sure we're not the only Windows client either.

Do you not bootstrap with clang-cl after building with MSVC? I'd expect that might help at least put you on the same performance tradeoffs as other users, if not be outright better.

[jh7370]

Do we care about performance on MSVC ?

Just responding to this point (although I accept it's moot in the current context): yes we care about performance when building with MSVC - our downstream toolchain is hosted on Windows. I'm sure we're not the only Windows client either.

Do you not bootstrap with clang-cl after building with MSVC? I'd expect that might help at least put you on the same performance tradeoffs as other users, if not be outright better.

I believe so (I'm not involved with the details of the full build process, so not entirely sure how it all fits together), but it still impacts how long the bootstrap build would take, plus internal users (like myself) don't always do the 2 stage build, since we just want a working compiler. Anyway, I'm sure there are others out there who don't do the bootstrap build for one reason or another.

Anyway, my main point was that there are people who care about performance on Windows, so please don't treat it as a second-class citizen.

[dwblaikie]

Anyway, my main point was that there are people who care about performance on Windows, so please don't treat it as a second-class citizen.

Yeah, I don't want to treat folks on Windows as second class citizens by any means - but I wouldn't mind/hope we can treat folks who care about performance but don't build their performance-critical binaries with clang, on platforms clang supports, as a lower priority. Yeah, don't make the build unusable when built without clang, but I don't think/hope we don't need to try too hard to get every ounce of performance out of every host compiler when/where that performance can be obtained with a bootstrap. Helps avoid us bending over backwards for compilers we can't fix, etc. Compared to making better tradeoffs between source workarounds and compiler improvements wholely within our control.

[AaronBallman]

Anyway, my main point was that there are people who care about performance on Windows, so please don't treat it as a second-class citizen.

Yeah, I don't want to treat folks on Windows as second class citizens by any means - but I wouldn't mind/hope we can treat folks who care about performance but don't build their performance-critical binaries with clang, on platforms clang supports, as a lower priority.

Consider testing as a counter example. We don't seriously expect anyone to do a bootstrap build each time they want to run their tests and folks do still run tests locally on their machine, and may run those tests frequently.

[dwblaikie]

Anyway, my main point was that there are people who care about performance on Windows, so please don't treat it as a second-class citizen.

Yeah, I don't want to treat folks on Windows as second class citizens by any means - but I wouldn't mind/hope we can treat folks who care about performance but don't build their performance-critical binaries with clang, on platforms clang supports, as a lower priority.

Consider testing as a counter example. We don't seriously expect anyone to do a bootstrap build each time they want to run their tests and folks do still run tests locally on their machine, and may run those tests frequently.

Not each time - and I'm not suggesting performance should be horrendous when building with the platform's toolchain. Just that the last bit of performance may not be available with the platform's toolchain, and you might need to use clang.
I'd generally recommend folks use clang for developing with LLVM anyway, for diagnostic parity, etc - doesn't require a bootstrap every time, take a recent release, or when you setup on a new machine, build clang then use that for your development.
Yes, every now and then you might miss out on some perf improvement and rebuilding your clang toolchain used for building may help a bit - but nothing drastic.

[AaronBallman]

Just that the last bit of performance

Maybe we're talking about different situations; I may have missed some context in the discussion of this PR. I didn't see this as being about the last bit of performance. It sounded like this was going to knowingly introduce a performance regression in an popular configuration (RelWithDebInfo) for developing Clang because it made for cleaner code. That's not a good tradeoff IMO. That said, different contexts might find that tradeoff worthwhile. I just think we need to be careful -- developing LLVM is already frustrating to some folks due to our resource requirements and I have anecdotal evidence that it has prevented us from gaining new contributors, so I worry about steps which make that situation worse. If it benefits end users, that's one thing. But this sounded like it primarily only benefits ourselves, which is a harder sell to me.

I'd generally recommend folks use clang for developing with LLVM anyway

FWIW, that's not what I recommend. I recommend folks use whatever (supported) development environment meets their needs because that gets us the most real-world experience with things like how Clang compares to other toolchains in terms of features and behavior. I use Visual Studio as my daily driver and that has led to a number of improvements in Clang over the years, both in terms of language extension compatibility and in terms of other compiler features. (Not to suggest you are wrong to recommend using Clang! Just that it's a slightly different way of looking at how we get contributions.)

[dwblaikie]

Just that the last bit of performance

Maybe we're talking about different situations; I may have missed some context in the discussion of this PR. I didn't see this as being about the last bit of performance. It sounded like this was going to knowingly introduce a performance regression in an popular configuration (RelWithDebInfo) for developing Clang because it made for cleaner code. That's not a good tradeoff IMO. That said, different contexts might find that tradeoff worthwhile. I just think we need to be careful -- developing LLVM is already frustrating to some folks due to our resource requirements and I have anecdotal evidence that it has prevented us from gaining new contributors, so I worry about steps which make that situation worse. If it benefits end users, that's one thing. But this sounded like it primarily only benefits ourselves, which is a harder sell to me.

Looks like we were talking about whether MSVC constant evaluated strlen & whether we should workaround it not doing that.

It's a bit of an abstract discussion - since it seems the premise isn't true (MSVC did manage to compile the strlen to a constant in at least a simple example linked on godbolt earlier).

But even if it were true - I think there's some line where we'd say the extra performance on MSVC isn't worth the extra complexity to the codebase. I guess we don't have any concrete numbers on the abstract question (what such a missed optimization would cost, since it doesn't appear to exist in reality) - but my balance for that is fairly strongly tipped in favor of avoiding that complexity and for folks building LLVM with MSVC to see some perf cost for it. How much perf cost is too much? How much code complexity is too much? Don't know.

I'd generally recommend folks use clang for developing with LLVM anyway

FWIW, that's not what I recommend. I recommend folks use whatever (supported) development environment meets their needs because that gets us the most real-world experience with things like how Clang compares to other toolchains in terms of features and behavior. I use Visual Studio as my daily driver and that has led to a number of improvements in Clang over the years, both in terms of language extension compatibility and in terms of other compiler features. (Not to suggest you are wrong to recommend using Clang! Just that it's a slightly different way of looking at how we get contributions.)

I'm mostly OK with that too (though you can use clang in VS, right?) but hope we don't end up complicating the codebase substantially to improve performance in that situation. That it works, yes, important, that it performs usably, yes, important.

Appreciate the perspectives, though - bit surprised/good to understand the diversity of perspectives.

[AaronBallman]

Just that the last bit of performance

Maybe we're talking about different situations; I may have missed some context in the discussion of this PR. I didn't see this as being about the last bit of performance. It sounded like this was going to knowingly introduce a performance regression in an popular configuration (RelWithDebInfo) for developing Clang because it made for cleaner code. That's not a good tradeoff IMO. That said, different contexts might find that tradeoff worthwhile. I just think we need to be careful -- developing LLVM is already frustrating to some folks due to our resource requirements and I have anecdotal evidence that it has prevented us from gaining new contributors, so I worry about steps which make that situation worse. If it benefits end users, that's one thing. But this sounded like it primarily only benefits ourselves, which is a harder sell to me.

Looks like we were talking about whether MSVC constant evaluated strlen & whether we should workaround it not doing that.

It's a bit of an abstract discussion - since it seems the premise isn't true (MSVC did manage to compile the strlen to a constant in at least a simple example linked on godbolt earlier).

But even if it were true - I think there's some line where we'd say the extra performance on MSVC isn't worth the extra complexity to the codebase. I guess we don't have any concrete numbers on the abstract question (what such a missed optimization would cost, since it doesn't appear to exist in reality) - but my balance for that is fairly strongly tipped in favor of avoiding that complexity and for folks building LLVM with MSVC to see some perf cost for it. How much perf cost is too much? How much code complexity is too much? Don't know.

Yeah, I suspect reasonable people have different answers for that, too. I think we're not too far off in what we're hoping for though, which is basically for the benefits to outweigh the costs.

I'd generally recommend folks use clang for developing with LLVM anyway

FWIW, that's not what I recommend. I recommend folks use whatever (supported) development environment meets their needs because that gets us the most real-world experience with things like how Clang compares to other toolchains in terms of features and behavior. I use Visual Studio as my daily driver and that has led to a number of improvements in Clang over the years, both in terms of language extension compatibility and in terms of other compiler features. (Not to suggest you are wrong to recommend using Clang! Just that it's a slightly different way of looking at how we get contributions.)

I'm mostly OK with that too (though you can use clang in VS, right?) but hope we don't end up complicating the codebase substantially to improve performance in that situation. That it works, yes, important, that it performs usably, yes, important.

You can use Clang in Visual Studio, but I specifically want to use cl so that I see what new diagnostics they're issuing, what new weird codegen starts happening, that sort of thing. Basically, it helps with test coverage in various ways. But I think the ship has sailed about not complicating the codebase because of toolchain-specific performance behavior. One prime example is with bit-field packing. In Clang, we use unsigned for all bit-fields because cl doesn't pack adjacent fields together unless they have the same type. So an int and a bool and an enumeration as the underlying type for adjacent bit-fields causes problems. This makes the debugging experience worse for everyone, but to counter that, we came up with the LLVM_PREFERRED_TYPE macro (which hides a Clang-specific attribute we implemented for this case) to try to recapture some of that debugging aid for some folks. That said, I think we agree that we don't want to do that sort of thing too often -- it just happens that the tradeoffs in this case are worth the pain.

Appreciate the perspectives, though - bit surprised/good to understand the diversity of perspectives.

100% agreed, this is a great discussion!