Move the details of C99 identifier mangling out of ProjectModel
#544
Conversation
|
I wonder if this should be in utilities |
|
|
||
| /// Creates a string that contains only value C99 characters. | ||
| /// FIXME: We will likely want a better name... | ||
| public func mangledForC99() -> String { |
There was a problem hiding this comment.
I think this should probably be named to follow "C99ExtendedIdentifier" which is the technical standard it follows.
|
The previous version would throw if the result was an empty string, because it doesn't mangle the first character (IIRC)? |
|
@aciidb0mb3r Yes, |
|
@ddunbar I addressed this in the commit message actually. It looked like the code had originally been written to potentially remove invalid characters, which could lead to an empty string even when the input wasn't empty. But at least in the sate of the code as I found it, that couldn't happen, since it always replaced unsupported scalars with |
|
And as I mentioned, I don't think it would actually ever throw, because it no longer makes the string shorter. |
|
Ah, I see your point. I do remember it doing that at one point, it must have changed (for all I know it might have been me that changed it at some point). |
|
@aciidb0mb3r @ddunbar Do you think this should be in |
|
I do think this should be in Utilities because this is not general enough for basic. I am not sure about shell escaping, It feels like a Basic thing but I wouldn't mind it being in utilities. |
|
I agree Utilities seems better although I would feel much happier about that if we had a good name for Utilities. |
|
Ok. I will move this to |
|
This got stalled, going to make the discussed changes, test, and merge this now. I am not going to rename |
|
@swift-ci please test |
|
(that test was requested to see if the CI incremental-build changes are working — I know this still needs to be rebased) |
|
@swift-ci please test linux |
|
@swift-ci please test linux platform |
|
@swift-ci please test |
abertelrud
force-pushed
the
factor-out-c99-identifier-mangling
branch
from
2da3d8f to
f7fd7a1
Compare
|
@swift-ci please test |
|
I've moved it to |
|
Hmm, those failures suggest that not all the diffs got pushed to the PR (I specifically fixed the problem it's erroring out about). Investigating. |
… down into `Utility`. It looks as if at some point this was meant to be able to fail because it might remove all the characters and end up with an empty string, but now that it replaces characters it doesn't seem possible for it to end up with an empty string assuming it was given a non-empty string. Instead we just define it to never generate an empty string if given a non-empty string. It will continue to be the caller's responsibility to check separately for non-empty strings, for which it is likely to want different errors anyway (e.g. reporting a missing module name as different from a malformed module name or whatever).
abertelrud
force-pushed
the
factor-out-c99-identifier-mangling
branch
from
f7fd7a1 to
24aaf6e
Compare
|
Turns out I hadn't pushed the latest changes. Testing again. |
|
@swift-ci please test |
|
The Linux failure is puzzling, since apparently the build and the tests all succeeded. The failure seems to be in the automation that happens after that. |
|
I'll do some more local testing before merging. |
| XCTAssertEqual("1".mangledToC99ExtendedIdentifier(), "_") | ||
| XCTAssertEqual("1foo".mangledToC99ExtendedIdentifier(), "_foo") | ||
|
|
||
| // FIXME: There are lots more interesting test cases to add here. |
There was a problem hiding this comment.
Finally a chance to use emoji in swiftpm source code 😀
Move the details of C99 identifier mangling out of
ProjectModeland down intoBasic. It looks as if at some point this was meant to be able to fail because it might remove all the characters and end up with an empty string, but now that it replaces characters it doesn't seem possible for it to end up with an empty string assuming it was given a non-empty string.