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

Improve string names for locally-bound Heapster variables #1399

Merged
merged 3 commits into from
Jul 29, 2021

Conversation

eddywestbrook
Copy link
Contributor

Currently, whenever a new local name is bound in Heapster, it is called either "zNNN" or "xNNN", depending on how it gets introduced, where "NNN" is a unique number. This PR changes that so that the variable name starts with a prefix which matches the type; e.g., LLVM pointers become ptrNNN, bitvectors become bvNNN, etc.

Eddy Westbrook added 2 commits July 28, 2021 12:27
@eddywestbrook eddywestbrook added the subsystem: heapster Issues specifically related to memory verification using Heapster label Jul 29, 2021
@eddywestbrook eddywestbrook requested a review from glguy July 29, 2021 16:51
Copy link
Member

@glguy glguy left a comment

Choose a reason for hiding this comment

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

The change looks good.

Things worth considering, but not blockers:

  • When setVarType is given a (Just str), use that as a prefix/suffix along with the typeBaseName so that top, local, and ghosts benefit from their type-based name-hint as well.
  • It's probably time to start putting {- ^ ... -} -> haddock comments on the type signatures with multiple Maybe String arguments.

@eddywestbrook eddywestbrook requested review from glguy and m-yac July 29, 2021 18:28
@eddywestbrook
Copy link
Contributor Author

Awesome, thanks Eric! I'll implement those changes.

Copy link
Contributor

@m-yac m-yac left a comment

Choose a reason for hiding this comment

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

Looks good to me! I agree with Eric's comment about adding comments.

I have one very unimportant observation: The error case of ppInfoAddExprName seems slightly strange to me. At the moment it will never be hit because it's always passed the result of typeBaseName, but if in the future we include user-defined strings in typeBaseName and one of those strings happens to have a number at the end, we probably don't want to error, I suspect we really just want to use some default name.

@eddywestbrook
Copy link
Contributor Author

@m-yac: I see what you are saying, but I don't actually expect those to ever be user-defined strings. The only place that could possibly happen would be from the C variable names, which always end with a close bracket, i.e., always look like C[xxx].

@eddywestbrook
Copy link
Contributor Author

Merging manually, because the only changes since the CI succeeded last time are additional comments and a slight tweak to what strings are printed.

@eddywestbrook eddywestbrook merged commit da7d859 into master Jul 29, 2021
@mergify mergify bot deleted the heapster-improved-var-names branch July 29, 2021 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
subsystem: heapster Issues specifically related to memory verification using Heapster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants