-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Typos (misc): cmake
, tools
, utils
, unittests
, validation-test
+ more
#75034
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.
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
Typos (misc): cmake
, tools
, utils
, unittests
, validation-test
+ more
#75034
Conversation
@@ -313,19 +313,19 @@ fileprivate class ConcurrencyDumper { | |||
let taskToThread: [swift_addr_t: UInt64] = | |||
Dictionary(threadCurrentTasks.map{ ($1, $0) }, uniquingKeysWith: { $1 }) | |||
|
|||
var lastChilds: [Bool] = [] | |||
var lastChildren: [Bool] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While grammatically incorrect, I think that in general, this type of change is less desirable as it is changing terminology for variables rather than correcting an obvious typo. In this specific case, the rename is fine, but it is not always so obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. This array isn't an array of children. It's an array of "last child" flags. lastChildren
is wrong. A better name might be lastChildFlags
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
@@ -64,6 +64,6 @@ TEST_F(ScanTest, TestDoesNotHaveArgumentQuery) { | |||
bool hasOption; | |||
hasOption = optionSet.find("-clearly-not-a-compiler-flag") != optionSet.end(); | |||
EXPECT_EQ(hasOption, false); | |||
hasOption = optionSet.find("-emit-modul") != optionSet.end(); | |||
hasOption = optionSet.find("-emit-module") != optionSet.end(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this has never triggered an error? Seems like we have insufficient test coverage :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is intentional, as it's followed by EXPECT_EQ(hasOption, false);
and this test seems to be about arguments that don't exist. I assume it's meant to be very close to a real argument name, maybe to ensure nothing is doing a prefix match or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikeash makes sense, I will revert the change then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikeash fixed!
@@ -9,8 +9,8 @@ extension TestLayout { | |||
return f() | |||
} | |||
} | |||
struct EqualWitdthHStack : TestLayout {} | |||
extension EqualWitdthHStack: View { | |||
struct EqualWidthHStack : TestLayout {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, these types of changes should be avoided IMO. In this specific case, this is a test program so it doesn't matter, but this runs the risk of an ABI break.
@swift-ci please test |
Co-authored-by: Saleem Abdulrasool <compnerd@compnerd.org>
@swift-ci please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lastChildren
should be lastChildFlags
instead, I think. Or just leave it alone.
@@ -313,19 +313,19 @@ fileprivate class ConcurrencyDumper { | |||
let taskToThread: [swift_addr_t: UInt64] = | |||
Dictionary(threadCurrentTasks.map{ ($1, $0) }, uniquingKeysWith: { $1 }) | |||
|
|||
var lastChilds: [Bool] = [] | |||
var lastChildren: [Bool] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. This array isn't an array of children. It's an array of "last child" flags. lastChildren
is wrong. A better name might be lastChildFlags
.
@swift-ci Please test |
utils/line-directive
Outdated
@@ -433,7 +433,7 @@ def run(): | |||
... // ###sourceLocation(file: "/public/core/Map.swift.gyb", line: 108) | |||
... | |||
... /// A `Collection` whose elements consist of those in a `Base` | |||
... /// `Collection` passed through a transform function returning `Elemen | |||
... /// `Collection` passed through a transform function returning `Element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the closing "`" is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
utils/line-directive
Outdated
@@ -471,7 +471,7 @@ def run(): | |||
... // ###sourceLocation(file: "/public/core/Map.swift.gyb", line: 108) | |||
... | |||
... /// A `Collection` whose elements consist of those in a `Base` | |||
... /// `Collection` passed through a transform function returning `Elemen | |||
... /// `Collection` passed through a transform function returning `Element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
utils/line-directive
Outdated
@@ -509,7 +509,7 @@ def run(): | |||
... // ###sourceLocation(file: "/public/core/Map.swift.gyb", line: 108) | |||
... | |||
... /// A `Collection` whose elements consist of those in a `Base` | |||
... /// `Collection` passed through a transform function returning `Elemen | |||
... /// `Collection` passed through a transform function returning `Element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found one that I think is supposed to be misspelled. Everything else looks good.
@@ -64,6 +64,6 @@ TEST_F(ScanTest, TestDoesNotHaveArgumentQuery) { | |||
bool hasOption; | |||
hasOption = optionSet.find("-clearly-not-a-compiler-flag") != optionSet.end(); | |||
EXPECT_EQ(hasOption, false); | |||
hasOption = optionSet.find("-emit-modul") != optionSet.end(); | |||
hasOption = optionSet.find("-emit-module") != optionSet.end(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is intentional, as it's followed by EXPECT_EQ(hasOption, false);
and this test seems to be about arguments that don't exist. I assume it's meant to be very close to a real argument name, maybe to ensure nothing is doing a prefix match or something like that?
@swift-ci Please test |
Fix typos in misc folders:
cmake
,tools
,utils
,unittests
,validation-test
+ moreThis is one batch of many PRs fixing typos, see the tracking issue.