ABI FIXME#16,17: Hoist += operator out of the array types into RangeReplaceableCollection #5170

airspeedswift · 2016-10-07T00:49:16Z

This was blocked on a compiler crash that is since fixed (rdar://problem/16566712).

May have performance impact so just putting it here to test/review for now.

airspeedswift · 2016-10-07T00:49:29Z

@swift-ci Please smoke test

Gankra · 2016-10-07T15:47:35Z

Looks like this change is making += ambiguous.

airspeedswift · 2016-10-07T16:02:12Z

Yeah looking at that now.

moiseev · 2016-10-07T18:14:06Z

I would guess that making it static func += in the NewArray should resolve the ambiguity. It should be done anyway because SE-0091.

…blgen-dep

When building on a macOS host, and when `SWIFT_INCLUDE_TESTS` is specified, the `swiftSwiftReflectionTest` target is added to all platforms. However, this target has a dependency upon Foundation, which is not available on non-Apple platforms. Use `add_swift_library`'s `TARGET_SDKS` parameter and other gating logic to ensure the target is only added for platforms that actually have Darwin available.

This approach side steps the whole dependency issue between the lipo target and the codesign target by just combining them (if needed). This is accomplished by moving the codesign code into `_add_swift_lipo_target` and having it conditional on passing `CODESIGN` as an option. This patch also updates `_add_swift_lipo_target` to use named arguments instead of positional ones, and provides a little bit of cleanup. There is more opportunity for larger refactoring and cleanup here by not generating a lipo target at all on non-darwin platforms.

This code was added in the mega-commit 6670bb7 with the commit message "Rewrite the CMake build system", so I'm a little bit light on context here. The comment in the code was: # Construct a unique name for the custom target. # Use a hash so that the file name does not push the OS limits for filename # length. This seems bunk to me. Since the string being hashed is a filename it seems to me that we don't need a hash to stay under the OS filename size limits. Additionally since output files must be unique using the filename as the unique target name seems like a good idea to me. The only issue here is that custom target names can't contain '/'s. To workaround that we replace the '/' character with '-'. This patch has the added benefit of having the full filename encoded into the target names, so if you need to debug the build dependency graph you can much more easily walk back from the generated build files to the place in CMake where it was generated.

…nce methods We strip the first input type on instance methods like, struct S { func foo(T) -> U } // (S)->(T)->U but we should not do that when it's actually a curried instance method, such as S.foo.

... instead of ManagedBufferPointer. This is what we already did for Array, Set and Dictionary. The intention is to simplify the generated SIL which is generated for ManagedBuffer operations.

airspeedswift · 2016-10-07T22:24:27Z

Yeah it does. But in fact shouldn't the protocol requirement for += just be removed? The generic versions are implemented in terms of methods on RangeReplaceableCollectionType but it isn't a requirement of the protocol that you implement it.

On Darwin, LLVM builds with -gmodules which causes debug info for the types defined by the LLVM modules is stored alongside the modules in the module cache. Deleting the module cache on every re-invocation of build-script without re-building the LLVM and clang modules will erase the debug info and thus render any LLVM types undebuggable. rdar://problem/28655564

…MyArrayProtocol

airspeedswift · 2016-10-07T22:43:12Z

@swift-ci Please smoke test

moiseev · 2016-10-07T22:44:15Z

I think you're right. Since += is already on the RangeReplaceable, the += requirement on MyArrayProtocol should no longer be necessary.

[code-completion] Fix type-relation check on implicitly curried instance methods

airspeedswift · 2016-10-07T22:47:20Z

@swift-ci Please smoke test

swift-ci · 2016-10-07T22:52:12Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 96c87d1
Test requested by - @airspeedswift

moiseev · 2016-10-07T22:53:28Z

OK. I see what you mean now. Removing += as there is already an append(contentsOf:). But this is exactly what the original message in the code comment said =) It should be an operator, not a method. Unfortunately, now it would require an evolution proposal.
I think it's worth adding back a FIXME(ABI): this should be an operator.

swift-ci · 2016-10-07T22:58:25Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 96c87d1
Test requested by - @airspeedswift

Use a SmallVector for scratch space to use to build the ArrayRefs instead.

airspeedswift · 2016-10-07T23:11:48Z

Oh I see, as in there shouldn't be a .append(contentsof:) method, only +=. Hmm.

Is that compatible with having two different versions of +=, for sequences and collections? (though IIRC there's another FIXME about that somewhere else, but I can't find it right now).

I can see it being preferable having it still be a method, to go along with insert and remove rather than this particular one being an operator. Also that would leave .append(newElement:) unless that also had an overload for +=.

So yeah, needs an evolution proposal (probably did just for moving them up from Array I guess)

moiseev · 2016-10-07T23:18:51Z

Right now there is no difference in the implementation of append(contentsOf:) for Sequence or Collection rhs. Internally it calls underestimatedCount to estimate the future capacity, but that property for a collection returns the real count, so should be OK.

I don't think evolution proposal is required for the change you've made, as it simply moves the method from a concrete type to a protocol, which is additive.

Solving the problem or protocol requirement (whether it should be an operator or a method) is not part of this change, so I still think adding (or rather keeping) a FIXME would be nice.

airspeedswift · 2016-10-07T23:26:23Z

Ah so in fact the two implementations of += (that I just cut/pasted from Array) were pointless? In which case may as well remove the collection one I guess.

[CMake] Fix codesign dependency (take 2)

[CMake] Don't use hashes in target names

stdlib: Use new built-ins for tail-allocated arrays in ManagedBuffer

swiftlang#5284)

[Parse] Improve error handling in parseList

…ookupConformance(swift::Type, swift::ProtocolDecl*, swift::LazyResolver*) Stack trace: ``` found code completion token A at offset 137 swift-ide-test: /path/to/swift/lib/AST/Module.cpp:596: ArrayRef<swift::Substitution> swift::TypeBase::gatherAllSubstitutions(Module *, swift::LazyResolver *, swift::DeclContext *): Assertion `boundGeneric->getGenericArgs().size() == genericSig->getInnermostGenericParams().size()' failed. 8 swift-ide-test 0x0000000000cad47d swift::ModuleDecl::lookupConformance(swift::Type, swift::ProtocolDecl*, swift::LazyResolver*) + 1677 9 swift-ide-test 0x0000000000a706ce swift::TypeChecker::conformsToProtocol(swift::Type, swift::ProtocolDecl*, swift::DeclContext*, swift::OptionSet<swift::ConformanceCheckFlags, unsigned int>, swift::ProtocolConformance**, swift::SourceLoc) + 62 10 swift-ide-test 0x0000000000a9e689 swift::TypeChecker::resolveTypeInContext(swift::TypeDecl*, swift::DeclContext*, swift::OptionSet<swift::TypeResolutionFlags, unsigned int>, bool, swift::GenericTypeResolver*, llvm::function_ref<bool (swift::TypeCheckRequest)>*) + 1273 14 swift-ide-test 0x0000000000a9fdfd swift::TypeChecker::resolveIdentifierType(swift::DeclContext*, swift::IdentTypeRepr*, swift::OptionSet<swift::TypeResolutionFlags, unsigned int>, bool, swift::GenericTypeResolver*, llvm::function_ref<bool (swift::TypeCheckRequest)>*) + 157 16 swift-ide-test 0x0000000000aa0e0f swift::TypeChecker::resolveType(swift::TypeRepr*, swift::DeclContext*, swift::OptionSet<swift::TypeResolutionFlags, unsigned int>, swift::GenericTypeResolver*, llvm::function_ref<bool (swift::TypeCheckRequest)>*) + 159 17 swift-ide-test 0x0000000000a9f5a5 swift::TypeChecker::validateType(swift::TypeLoc&, swift::DeclContext*, swift::OptionSet<swift::TypeResolutionFlags, unsigned int>, swift::GenericTypeResolver*, llvm::function_ref<bool (swift::TypeCheckRequest)>*) + 277 18 swift-ide-test 0x0000000000a1ff62 swift::TypeChecker::checkInheritanceClause(swift::Decl*, swift::GenericTypeResolver*) + 2178 19 swift-ide-test 0x0000000000a62cd6 swift::TypeChecker::checkGenericParamList(swift::ArchetypeBuilder*, swift::GenericParamList*, swift::GenericSignature*, swift::GenericEnvironment*, swift::GenericTypeResolver*) + 326 20 swift-ide-test 0x0000000000a640c4 swift::TypeChecker::validateGenericSignature(swift::GenericParamList*, swift::DeclContext*, swift::GenericSignature*, bool, std::function<void (swift::ArchetypeBuilder&)>) + 116 21 swift-ide-test 0x0000000000a64b10 swift::TypeChecker::validateGenericTypeSignature(swift::GenericTypeDecl*) + 128 22 swift-ide-test 0x0000000000a21bac swift::TypeChecker::validateDecl(swift::ValueDecl*, bool) + 348 27 swift-ide-test 0x0000000000a28126 swift::TypeChecker::typeCheckDecl(swift::Decl*, bool) + 150 28 swift-ide-test 0x0000000000a4e64f swift::performTypeChecking(swift::SourceFile&, swift::TopLevelContext&, swift::OptionSet<swift::TypeCheckingFlags, unsigned int>, unsigned int, unsigned int) + 1055 29 swift-ide-test 0x0000000000838ef6 swift::CompilerInstance::performSema() + 3350 30 swift-ide-test 0x00000000007d9854 main + 42916 Stack dump: 0. Program arguments: swift-ide-test -code-completion -code-completion-token=A -source-filename=<INPUT-FILE> 1. While type-checking 'c' at <INPUT-FILE>:3:1 2. While resolving type T at [<INPUT-FILE>:3:25 - line:3:25] RangeText="T" ```

…wift-moduledecl-lookupconformance

Fixed an assert caused when a TupleExpr that didn't have a valid SourceRange had a valid SourceLoc for the first element but not for the last

…Names. 'processInfo' was removed entirely because it's now considered a class property, and we don't ever import those as initializers.

…typeCheckAbstractFunctionBodyUntil(swift::AbstractFunctionDecl*, swift::SourceLoc) Stack trace: ``` found code completion token A at offset 141 swift-ide-test: /path/to/swift/lib/Sema/TypeCheckStmt.cpp:1277: bool swift::TypeChecker::typeCheckFunctionBodyUntil(swift::FuncDecl *, swift::SourceLoc): Assertion `BS && "Should have a body"' failed. 8 swift-ide-test 0x0000000000a98477 swift::TypeChecker::typeCheckAbstractFunctionBodyUntil(swift::AbstractFunctionDecl*, swift::SourceLoc) + 39 9 swift-ide-test 0x0000000000a50491 swift::typeCheckAbstractFunctionBodyUntil(swift::AbstractFunctionDecl*, swift::SourceLoc) + 657 13 swift-ide-test 0x0000000000c5d4d4 swift::Decl::walk(swift::ASTWalker&) + 20 14 swift-ide-test 0x0000000000cb22ee swift::SourceFile::walk(swift::ASTWalker&) + 174 15 swift-ide-test 0x0000000000cb15ff swift::ModuleDecl::walk(swift::ASTWalker&) + 95 16 swift-ide-test 0x0000000000c87fc4 swift::DeclContext::walkContext(swift::ASTWalker&) + 180 17 swift-ide-test 0x0000000000989aa8 swift::performDelayedParsing(swift::DeclContext*, swift::PersistentParserState&, swift::CodeCompletionCallbacksFactory*) + 136 18 swift-ide-test 0x0000000000839051 swift::CompilerInstance::performSema() + 3697 19 swift-ide-test 0x00000000007d9854 main + 42916 Stack dump: 0. Program arguments: swift-ide-test -code-completion -code-completion-token=A -source-filename=<INPUT-FILE> 1. While walking into decl getter for a at <INPUT-FILE>:3:5 ```

[APINotes] Replace old FactoryAsInit annotation with equivalent SwiftNames.

…wift-typechecker-typecheckabstractfunctionbodyuntil

swiftlang#5280)

…ructions, just put the name in SILNodes.def and use metaprogramming. This is a cleanup for SILParsing/Printing. I verified that everything was spelled correctly by taking the current parsing switch moving that into a file, regenerating it using the .def file and then diffed them. The diff was the same. rdar://28685236

SR-1255: Improve diagnostic when one of the parameters marked as autoclosure

Always check arguments of the tuple type against corresponding parameters, otherwise for a single argument functions e.g. foo(_ a: Any) after SE-0046 type checker is going to produce incorrect solution.

…g#5062)

SR-2505: Fix "Call arguments did not match up" assertion

… search path.

…ing_parsing_instead_of_hardcoding_names [sil] Rather than maintaining manually the textual opcode for SILInst…

Initialize CachedVFile with nullptr

…KNodes. (swiftlang#5297) During the lifetime of analysing source-breaking changes, we may compare the equality of two SDKNodes more than once, which makes the comprison results amenable for caching. This patch implemented it. NFC

…MyArrayProtocol

…edswift/swift into range-replaceable-append-operator

Merge main 2023-01-11

swift-ci and others added 7 commits October 7, 2016 12:41

Merge pull request swiftlang#5178 from llvm-beanz/sil-missing-clang-t…

34d81d4

…blgen-dep

[code-completion] Fix type-relation check on implicitly curried insta…

a6934d1

…nce methods We strip the first input type on instance methods like, struct S { func foo(T) -> U } // (S)->(T)->U but we should not do that when it's actually a curried instance method, such as S.foo.

supress warnings on printing optionals

a7e1900

stdlib: Use new built-ins for tail-allocated arrays in ManagedBuffer

0aa023a

... instead of ManagedBufferPointer. This is what we already did for Array, Set and Dictionary. The intention is to simplify the generated SIL which is generated for ManagedBuffer operations.

adrian-prantl and others added 2 commits October 7, 2016 15:32

remove += operator from RangeReplaceableCollection and also the test …

418f335

…MyArrayProtocol

Merge pull request swiftlang#5183 from benlangmuir/cc-inst-curry-ref

114d836

[code-completion] Fix type-relation check on implicitly curried instance methods

jckarter and others added 2 commits October 7, 2016 16:04

Add a -disable-sil-partial-apply frontend staging option.

2a86a6b

Fix a couple cases of ArrayRef capturing compiler temps.

5356cc3

Use a SmallVector for scratch space to use to build the ArrayRefs instead.

airspeedswift and others added 4 commits October 7, 2016 16:35

Added FIXME(ABI) comment about replacing append with +=

89351a9

Merge pull request swiftlang#5181 from llvm-beanz/fix-codesign-dep-take2

cfbd5c3

[CMake] Fix codesign dependency (take 2)

Merge pull request swiftlang#5182 from llvm-beanz/dont-use-hashes

370697f

[CMake] Don't use hashes in target names

Merge pull request swiftlang#5153 from eeckstein/managedbuffer

b1d2d1a

stdlib: Use new built-ins for tail-allocated arrays in ManagedBuffer

nkcsgexi and others added 28 commits October 13, 2016 21:04

api-digester: teach the tool to detect new mutating/throwing functions (

edea597

swiftlang#5284)

Merge pull request swiftlang#5287 from gregomni/postfixOp

be55ede

Merge pull request swiftlang#5194 from rintaro/parselist-earlybailout

2cd25c8

[Parse] Improve error handling in parseList

Merge pull request swiftlang#5292 from practicalswift/sourcekit-106-s…

3f9d797

…wift-moduledecl-lookupconformance

Merge pull request swiftlang#5196 from mxswd/fix-tupleexpr-sourcerange

6ae16d0

Fixed an assert caused when a TupleExpr that didn't have a valid SourceRange had a valid SourceLoc for the first element but not for the last

[APINotes] Replace old FactoryAsInit annotation with equivalent Swift…

6163679

…Names. 'processInfo' was removed entirely because it's now considered a class property, and we don't ever import those as initializers.

Removed unused file that probably predates API notes.

37d3fe1

Merge pull request swiftlang#5285 from jrose-apple/modern-API-notes

41a82ef

[APINotes] Replace old FactoryAsInit annotation with equivalent SwiftNames.

Merge pull request swiftlang#5293 from practicalswift/sourcekit-107-s…

3cbdcef

…wift-typechecker-typecheckabstractfunctionbodyuntil

Clean up swift-api-digester: Remove dead code. Fix typos. Fix headers. (

6a61476

swiftlang#5280)

Merge pull request swiftlang#5239 from xedin/SR-1255

32634b1

SR-1255: Improve diagnostic when one of the parameters marked as autoclosure

[Type Checker] SR-2505: Fix "Call arguments did not match up" assertion

578e36a

Always check arguments of the tuple type against corresponding parameters, otherwise for a single argument functions e.g. foo(_ a: Any) after SE-0046 type checker is going to produce incorrect solution.

[stdlib] Reorder increment and element creation to not need (swiftlan…

3d7f8cc

…g#5062)

Merge pull request swiftlang#5286 from xedin/SR-2505-v2

3a493b2

SR-2505: Fix "Call arguments did not match up" assertion

[swift-api-digester] Add option to be able to pass a system framework…

54163ee

… search path.

Initialize CachedVFile with nullptr

09cbffb

Merge pull request swiftlang#5294 from gottesmm/use_silnode_for_print…

b3dc093

…ing_parsing_instead_of_hardcoding_names [sil] Rather than maintaining manually the textual opcode for SILInst…

Merge pull request swiftlang#5296 from apple/ubuntu-16-10-fix

e250960

Initialize CachedVFile with nullptr

Hoist += operator out of the array types into RangeReplaceableCollection

b032617

supress warnings on printing optionals

12e3c97

remove += operator from RangeReplaceableCollection and also the test …

240519f

…MyArrayProtocol

Added FIXME(ABI) comment about replacing append with +=

f5f2ecd

Remove comment re: since-added + operators

fbf1809

Merge branch 'range-replaceable-append-operator' of github.com:airspe…

4c797da

…edswift/swift into range-replaceable-append-operator

airspeedswift closed this Oct 15, 2016

MaxDesiatov pushed a commit that referenced this pull request Sep 7, 2023

Merge pull request #5170 from swiftwasm/katei/merge-main-2023-01-11

4873245

Merge main 2023-01-11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ABI FIXME#16,17: Hoist += operator out of the array types into RangeReplaceableCollection #5170

ABI FIXME#16,17: Hoist += operator out of the array types into RangeReplaceableCollection #5170

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

Gankra commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

moiseev commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

moiseev commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

swift-ci commented Oct 7, 2016

Uh oh!

moiseev commented Oct 7, 2016

Uh oh!

swift-ci commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016 •

edited

Loading

Uh oh!

moiseev commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

Uh oh!

ABI FIXME#16,17: Hoist += operator out of the array types into RangeReplaceableCollection #5170

ABI FIXME#16,17: Hoist += operator out of the array types into RangeReplaceableCollection #5170

Uh oh!

Conversation

airspeedswift commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

Gankra commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

moiseev commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

moiseev commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

swift-ci commented Oct 7, 2016

Uh oh!

moiseev commented Oct 7, 2016

Uh oh!

swift-ci commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moiseev commented Oct 7, 2016

Uh oh!

airspeedswift commented Oct 7, 2016

Uh oh!

Uh oh!

airspeedswift commented Oct 7, 2016 •

edited

Loading