Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SR-8135] Changes for bug fix to do a clang check only when needed for certain … #1695

Merged
merged 2 commits into from Jul 24, 2018
Merged

[SR-8135] Changes for bug fix to do a clang check only when needed for certain … #1695

merged 2 commits into from Jul 24, 2018

Conversation

prachipai
Copy link
Contributor

…commands

@prachipai prachipai changed the title Changes for bug fix to do a clang check only when needed for certain … [SR-8135] Changes for bug fix to do a clang check only when needed for certain … Jul 23, 2018
ankitspd
ankitspd reviewed Jul 23, 2018
View reviewed changes
Sources/Commands/UserToolchain.swift Outdated
self.destination = destination

// Get the search paths from PATH.
let envSearchPaths = getEnvSearchPaths(
Copy link
Member

Choose a reason for hiding this comment

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

I think we should convert this into a property so we don't need to construct it again in the getClangCompiler method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ankitspd
ankitspd reviewed Jul 23, 2018
View reviewed changes
Sources/Commands/UserToolchain.swift Outdated
let envSearchPaths = getEnvSearchPaths(
pathString: getenv("PATH"), currentWorkingDirectory: localFileSystem.currentWorkingDirectory)

func lookup(fromEnv: String) -> AbsolutePath? {
Copy link
Member

Choose a reason for hiding this comment

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

Similarly this could be a private method in this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ankitspd
ankitspd reviewed Jul 23, 2018
View reviewed changes
Tests/BuildTests/BuildPlanTests.swift Outdated
@@ -32,6 +31,9 @@ private struct MockToolchain: Toolchain {
#else
let dynamicLibraryExtension = "so"
#endif
public func getClangCompiler() throws -> AbsolutePath {
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove public

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ankitspd
ankitspd reviewed Jul 23, 2018
View reviewed changes
Copy link
Member

@ankitspd ankitspd left a comment

Choose a reason for hiding this comment

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

Looks good, I added some minor comments.

ankitspd
ankitspd reviewed Jul 23, 2018
View reviewed changes
Sources/Commands/UserToolchain.swift Outdated
@@ -69,6 +66,9 @@ public struct UserToolchain: Toolchain {
/// The compilation destination object.
let destination: Destination

// Get the search paths from PATH
Copy link
Member

Choose a reason for hiding this comment

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

nit: /// Search paths from the PATH environment variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ankitspd
ankitspd reviewed Jul 23, 2018
View reviewed changes
Sources/Commands/UserToolchain.swift Outdated
private static func lookup(fromEnv: String, searchPaths: [AbsolutePath]) -> AbsolutePath? {
return lookupExecutablePath(
filename: getenv(fromEnv),
searchPaths: searchPaths)
Copy link
Member

Choose a reason for hiding this comment

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

nit: all this could be in one line.

return lookupExecutablePath(filename: getenv(fromEnv), searchPaths: searchPaths)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ankitspd
ankitspd reviewed Jul 23, 2018
View reviewed changes
Sources/Commands/UserToolchain.swift Outdated

// Get the binDir from destination.
let binDir = destination.binDir
private static func lookup(fromEnv: String, searchPaths: [AbsolutePath]) -> AbsolutePath? {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should rename "fromEnv" to "executable", so the signature becomes:

private static func lookup(executable: String, searchPaths: [AbsolutePath]) -> AbsolutePath?

Copy link
Member

Choose a reason for hiding this comment

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

actually, it should be "variable" not "executable".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ankitspd
ankitspd reviewed Jul 23, 2018
View reviewed changes
Sources/Commands/UserToolchain.swift Outdated
// Get the binDir from destination.
let binDir = destination.binDir

let swiftCompilers = try UserToolchain.determineSwiftCompilers(binDir: binDir, lookup: {UserToolchain.lookup(fromEnv: $0, searchPaths: searchPaths)})
Copy link
Member

Choose a reason for hiding this comment

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

nit: needs a space after { and before }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ankitspd
ankitspd reviewed Jul 23, 2018
View reviewed changes
Sources/Commands/UserToolchain.swift Outdated
@@ -69,6 +66,9 @@ public struct UserToolchain: Toolchain {
/// The compilation destination object.
let destination: Destination

/// Search paths from the PATH environment variable
Copy link
Member

Choose a reason for hiding this comment

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

nit: missing .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ankitspd
Copy link
Member

@swift-ci please smoke test

ankitspd
ankitspd approved these changes Jul 23, 2018
View reviewed changes
Copy link
Member

@ankitspd ankitspd left a comment

Choose a reason for hiding this comment

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

Thanks!

@ankitspd ankitspd merged commit 353522a into apple:master Jul 24, 2018
@ankitspd ankitspd mentioned this pull request Jul 24, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankitspd ankitspd ankitspd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@prachipai @ankitspd