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.

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

Fix Issue 22418 - Error in documentation on strings #3501

Merged
merged 12 commits into from Jan 31, 2023

Conversation

andraam21
Copy link
Contributor

No description provided.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 28, 2023

Thanks for your pull request and interest in making D better, @andraam21! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
22418 enhancement Error in documentation on strings

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

Please leave the indentation as is was

error at immutable strings at str1
@andraam21 andraam21 changed the title immutable strings Fix issue 22418 - Error in documentation on strings Jan 28, 2023
@RazvanN7
Copy link
Contributor

Please add a commit that contains "Fix Issue 22418" in the commit message so that dlang-bot auto-closes the issue when the PR is merged.

Copy link
Contributor Author

@andraam21 andraam21 left a comment

Choose a reason for hiding this comment

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

Fix Issue 22418

Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

The PR does not fix the issue. The OP asks for a clarification on what happens here and in its current form the PR does not address that. To be accepted, this needs to:

  • the comments must match the errors that the compiler outputs
  • the difference between different combinations of immutable(char)[] or immutable(char[]) must be highlighted.

spec/arrays.dd Outdated
@@ -1008,10 +1008,11 @@ $(H3 $(LNAME2 strings, Strings))
$(SPEC_RUNNABLE_EXAMPLE_FAIL
---------
char[] str1 = "abc"; // error, "abc" is not mutable
Copy link
Contributor

Choose a reason for hiding this comment

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

The bug report asks for a redesign of this example because currently the errors do not match. For example, the line above yields:

cannot implicitly convert expression `"abc"` of type `string` to `char[]`

spec/arrays.dd Outdated
immutable(char)[] str3 = "abc"; // ok
immutable(char)[] str4 = str1; // error, str4 is not mutable
immutable(char)[] str5 = str1.idup; // ok, make immutable copy
immutable(char)[] str4 = str2; // error, str4 is not mutable
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the error that the compiler outputs.

Added the compiler errors and the difference between immutable(char)[] and immutable(char[])
@andraam21 andraam21 changed the title Fix issue 22418 - Error in documentation on strings Fix Issue 22418 - Error in documentation on strings Jan 30, 2023
spec/arrays.dd Outdated
int[4] a = 42; // set all elements of a to 42

assert(a == [42, 42, 42, 42]);
---------
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be unrelated to this PR. Maybe you somehow screwed the rebase?

spec/arrays.dd Show resolved Hide resolved
spec/arrays.dd Outdated
int[4] a = 42; // set all elements of a to 42

assert(a == [42, 42, 42, 42]);
---------
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be deleted.

spec/arrays.dd Outdated
---------
)
$(P
The type immutable(char)[] represents an array of immutable chars. However, the reference to the string is
Copy link
Contributor

Choose a reason for hiding this comment

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

please highlight the compiler keyword : immutable(char)[], immutable string etc.

Highlighted the words and added the missing part
Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Squash the commits and we are ready to go

white spaces fixed
@RazvanN7 RazvanN7 merged commit 5c41a68 into dlang:master Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants