-
Notifications
You must be signed in to change notification settings - Fork 39
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
internal: consider column for TLineInfo
equality
#880
Conversation
==
TlineInfo exact match==
TlineInfo exact match
==
TlineInfo exact match==
TlineInfo exact match
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.
Since it's the same as the lifted, you can remove the manual ==
implementation as a whole.
Also, could you elaborate on what you mean by "stop historical un-aware problem"? Apart from the simplification, which makes sense, what is the reason for this change?
|
==
TlineInfo exact match==
, proc exactEquals for TlineInfo
While I think the change is good, I don't think that the previous |
while if user hover the |
Hm, I'm not sure I understand the relation this has to the equality operation only considering the line number. Could you elaborate? |
it's not a short path to track, how the user know the type have custom template `!=`*(x, y: untyped): untyped =
## Unequals operator. This is a shorthand for `not (x == y)`.
not (x == y) oh, it has to be template ? not a operator? |
Are you saying that To be clear, I'm only trying to understand your reasoning behind this change. |
am saying without editor support I didn't aware there's custom |
Hm, okay. In my opinion, that could have also been mitigated via a less impactful change (e.g., by moving the equality operator and In any way, have you verified that there are no usage sites left where the old equality behaviour is relied on? |
I've found references on |
For finding all usages or possible usage, you can annotate the old I've found two places where the changed
The second one is maybe minor (only increasing the amount of warnings produced), but the first one is not, as the profiler wants to associate the time-taken with a line, and not a precise line + column. |
nice! so if this going forward, only need fix |
Apart from renaming For this PR, please make sure:
|
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
The new PR message is much better than the previous one. However, there are still some things that I think need adjustments:
The title and summary should provide a broad overview of the impact and reasoning to both reviewers and future contributors. Language-level changes or changes to the user interface also usually go in this section. The details section is for going into detail on the impact, what exactly changed, why it changed, what alternatives were considered (if any), etc. |
updated, sorry for my pool English |
==
, proc exactEquals for TlineInfoTLineInfo
equality
No worries, I and others can help you with writing commit messages. I've updated the title and body according to my previous comments. In addition, the change doesn't only affect the performance of |
/merge |
Merge requested by: @zerbina Contents after the first section break of the PR description has been removed and preserved below: |
Summary
Change the default equality operation for
TLineInfo
to also considerthe column, effectively changing the meaning of
TLineInfo
to "sourceposition info". Places in the compiler that relied on the old behaviour
are adjusted.
In most cases, a
TLineInfo
is already treated as a "source position",so having the
==
operator reflect that makes sense. Thehash
procedure also already considered the column, too.
Details
The custom
==
implementation forTLineInfo
is removed, which meansthat the default implementation (value equality) is used.
exactEquals
is redundant now: its usages are replaced with
==
and the procedureitself is removed.
Changed usage sites
The VM profiler implementation (
vmprofiler
) relied on the old equalitybehaviour, as it uses
TLineInfo
as the key for a table that associatesprofiler data with a source line. Here,
TLineInfo
usage is replacedwith the new
SourceLinePosition
tuple, which is aTLineInfo
withoutthe column information.
For rendering the profiler data, a temporary
TLineInfo
instance iscreated, but with the column set to -1. This means that the text output
now links to the source line without including a column position.
Other relevant usage sites
There are two other places where the different equality behaviour
causes a visible change in compiler behaviour, but that are not
adjusted because the new behaviour is better:
nilcheck.derefWarning
):warnings are now reported one per line+column, instead of one per
line
msgs.getContext
, "instantiatedfrom" messages): if there are multiple surrounding instantiation
contexts on the same line (but in different columns), they are now all
shown, instead of only the first one