Skip to content

[clang][dataflow] Remove RecordValue.getLog() usage in HTMLLogger #65645

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 12, 2023

Conversation

kinu
Copy link
Contributor

@kinu kinu commented Sep 7, 2023

We can dump the same information from RecordStorageLocation.

Tested the behavior before and after patch, that generates the field values in the HTML
in both cases (and also made sure that removing the relevant code makes the field values
in the HTML go away)

We can dump the same information from RecordStorageLocation.

Tested the behavior before and after patch, that generates the
field values in the HTML in both cases
(And also made sure if we remove the code it goes away)
@kinu kinu requested a review from a team as a code owner September 7, 2023 17:26
@github-actions github-actions bot added the clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html label Sep 7, 2023
@kinu
Copy link
Contributor Author

kinu commented Sep 7, 2023

@martinboehme @Xazax-hun @ymand if anyone can review this small patch that removes one more dependency on RecordValue.getLoc()... thanks!

Copy link
Collaborator

@ymand ymand left a comment

Choose a reason for hiding this comment

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

Thanks!

@martinboehme martinboehme self-requested a review September 11, 2023 06:58
@martinboehme
Copy link
Contributor

Can you add a screenshot of what this looks like after the change?

@kinu
Copy link
Contributor Author

kinu commented Sep 11, 2023

Here's the screenshot for a simple code snippet
Btp9XprWFz4w2yQ (1)
'Without the patch' image is almost identical

You can see some more screenshots here:
https://docs.google.com/document/d/1gJqU5g7HCUifDUJBb5NzSBC0K-9W4nyZ3zxtpbTUDdk/view

@martinboehme
Copy link
Contributor

Here's the screenshot for a simple code snippet !
[snip]
You can see some more screenshots here: https://docs.google.com/document/d/1gJqU5g7HCUifDUJBb5NzSBC0K-9W4nyZ3zxtpbTUDdk/view

Thanks for this -- looks great! Just wanted to make sure the output makes sense in context.

@martinboehme martinboehme merged commit e791535 into llvm:main Sep 12, 2023
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
…vm#65645)

We can dump the same information from RecordStorageLocation.

Tested the behavior before and after patch, that generates the field
values in the HTML
in both cases (and also made sure that removing the relevant code makes
the field values
in the HTML go away)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants