Skip to content

Fix replace_symbolt for non-lvalues #2200

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

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

tautschnig
Copy link
Collaborator

Previously only literal constants were considered non-lvalues, which is insufficient as the unit test shows.

@kroening
Copy link
Member

This requires something entirely different.

This should not consider the "lvalue" case, i.e., replacing should always be done.

If this is to be used on the lhs of an assignment, or the like, then some other function, say replace_expr_lvalue, or whatnot, should be defined.

@tautschnig
Copy link
Collaborator Author

This should not consider the "lvalue" case, i.e., replacing should always be done.

Ok, but that's been in the code base for a fair while and is not what this PR newly introduces. I'm happy to fix it though, if I could please get comments on two distinct cases:

  1. The lhs of an assignment: arguably this could be done via a separate function rather than controlling it via a parameter as is done now.
  2. Within an expression, where an lvalue must be maintained (such as the argument to address-of). What would be a case where substitution should be performed even when that would result in an ill-formed expression?

@tautschnig tautschnig self-assigned this May 21, 2018
@tautschnig
Copy link
Collaborator Author

The unit test (first commit in here) has now been moved to #2724, I'll rebase once that's in.

@tautschnig
Copy link
Collaborator Author

This is now based on #2723 with only the very last commit being new. Marking as do-not-merge until #2723 is merged.

@tautschnig tautschnig force-pushed the lvalue-prop branch 2 times, most recently from 8c81f4f to 8e15bbb Compare August 31, 2018 13:22
Literal constants are not the only ones: for example, address-of isn't an lvalue
either. Make is_lvalue globally available to accomplish this.
@tautschnig
Copy link
Collaborator Author

@kroening This has now been rebased and I'd hope your concerns have been addressed (though mainly that happened in #2723).

Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

Looks sensible

@tautschnig tautschnig merged commit 251740d into diffblue:develop Sep 19, 2018
@tautschnig tautschnig deleted the lvalue-prop branch September 19, 2018 16:00
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.

5 participants