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

[SR-11889] Use a Located<T> throughout the compiler instead of std::pair<SourceLoc, T> #54305

Closed
typesanitizer opened this issue Dec 3, 2019 · 4 comments
Labels
compiler The Swift compiler in itself good first issue Good for newcomers improvement

Comments

@typesanitizer
Copy link

Previous ID SR-11889
Radar rdar://problem/57579815
Original Reporter @typesanitizer
Type Improvement
Status Closed
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Improvement, StarterBug
Assignee mkita (JIRA)
Priority Medium

md5: d65bfa4b042fd60690d622e6c0308dd8

Issue Description:

Using std::pair<SourceLoc, T> or std::pair<T, SourceLoc> for fields makes reading usage sites harder as one needs to keep track of the types of the first and second field. It would be good to have a currency type:

template<typename T>
struct Located<T> {
  SourceLoc loc;
  T item;
}

and use that consistently throughout the compiler. For example,

  typedef ArrayRef<std::pair<Identifier, SourceLoc>> AccessPathTy;

could be replaced with

  typedef ArrayRef<Located<Identifier>> AccessPathTy;

Q: Where should Located<T> be defined?
A: I think it should be defined in a new header, Basic/Located.h. Basic because we expect it to be used throughout the compiler, similar to SourceLoc.

Q: What implicit conversions should Located<T> have?
A: We probably don't want an implicit conversion from T because that makes it easier for someone to create a Located<T> that conceptually doesn't make sense. Similarly we probably don't want a conversion from Located<T> to T as that makes it easy to accidentally drop a SourceLoc.

Q: Which operators should be overloaded?
A: Probably just the shift operator to make things convenient for print debugging. We can add more stuff later if the usage sites make it seem like more operator overloads would be useful.

@typesanitizer
Copy link
Author

@swift-ci create

@typesanitizer
Copy link
Author

Feel free to @varungandhi-apple for review if you submit a PR before I get to this 🙂.

@typesanitizer
Copy link
Author

gomezd (JIRA User) are you working on this issue? mkita (JIRA User) has a PR up for this (as they marked themselves as the assignee) so you don't need to work on this. Feel free to pick another StarterBug which isn't already taken 🙂.

@typesanitizer
Copy link
Author

Merged PR 28643 with the fix by Maksim Kita.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler The Swift compiler in itself good first issue Good for newcomers improvement
Projects
None yet
Development

No branches or pull requests

1 participant