Skip to content

Conversation

augusto2112
Copy link

rdar://87889973

@augusto2112
Copy link
Author

swiftlang/swift#59802

@swift-ci test

@augusto2112
Copy link
Author

swiftlang/swift#59802

@swift-ci test macOS

@augusto2112
Copy link
Author

@swift-ci test macOS

@@ -623,8 +623,25 @@ GetObjectFileFormat(llvm::Triple::ObjectFormatType obj_format_type) {
return obj_file_format;
}

static llvm::SmallVector<llvm::StringRef, 1>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it work to say SmallVector<llvm::StringRef>?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, I chose 1 because I think in the vast majority of cases we'll only have one name per image.

virtual bool readELF(swift::remote::RemoteAddress ImageStart,
llvm::Optional<llvm::sys::MemoryBlock> FileBuffer) = 0;
find_section,
llvm::SmallVector<llvm::StringRef, 1> likely_module_names = {}) = 0;
Copy link

@kastiglione kastiglione Jul 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: SmallVector<T, 1> an established convention/idiom? The docs say:

Prefer to use ArrayRef<T> or SmallVectorImpl<T> as a parameter type.
(source)

In the absence of a well-motivated choice for the number of inlined elements N, it is recommended to use SmallVector<T>
(source)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated above, I chose 1 because I think in the vast majority of cases we'll only have one name per image, which I think is a reasonable assumption. Perhaps I should've passed SmallVectorImpl to the addImage functions, but I just merged the swift side of this patch 😕

@augusto2112
Copy link
Author

@swift-ci macOS

@egorzhdan
Copy link

swiftlang/swift#59945

@swift-ci please test macOS

@augusto2112
Copy link
Author

swiftlang/swift#59955

@swift-ci please test macOS

@augusto2112 augusto2112 merged commit d6b0284 into swiftlang:stable/20211026 Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants