Skip to content

Conversation

MathiasVP
Copy link
Contributor

The PR in #2724 became quite messy with commits, so here's a fresh PR that should make it easier to follow the structure!

@MathiasVP MathiasVP added the C++ label Feb 5, 2020
@MathiasVP MathiasVP marked this pull request as ready for review February 5, 2020 16:12
@MathiasVP MathiasVP requested review from a team as code owners February 5, 2020 16:12
Comment on lines 26 to 31
min(Language::Location l |
l = getAnInstruction().getLocation()
|
l
order by
l.getFile().getAbsolutePath(), l.getStartLine(), l.getStartColumn(), l.getEndLine(),
l.getEndColumn()
)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should exclude UnknownLocation if there are any expressions with known locations in the same GVN. Otherwise implicit this qualifiers on member accesses will cause the value number for the this pointer to have an UnknownLocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I've excluded UnknownLocations if there are any non-UnknownLocations now.

final Location getLocation() {
result =
rank[1](Location l |
min(Location l |
Copy link
Contributor

Choose a reason for hiding this comment

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

The same note applies re: UnknownLocation and implicit this qualifiers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@MathiasVP MathiasVP force-pushed the ir-gvn-ast-wrapper-fixup branch from 20a20d2 to c39ca7c Compare February 6, 2020 10:05
@MathiasVP MathiasVP force-pushed the ir-gvn-ast-wrapper-fixup branch from c39ca7c to 2017ca8 Compare February 6, 2020 14:58
@MathiasVP MathiasVP force-pushed the ir-gvn-ast-wrapper-fixup branch from 8162e87 to 538c2b2 Compare February 6, 2020 17:44
Copy link
Contributor

@rdmarsh2 rdmarsh2 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants