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

Allow adding comments to generated IR #963

Merged
merged 5 commits into from
Jul 11, 2023
Merged

Allow adding comments to generated IR #963

merged 5 commits into from
Jul 11, 2023

Conversation

apmasell
Copy link
Contributor

As titled.

Closes #962

@esc esc added this to the v0.41.0rc1 milestone Jun 27, 2023
@esc esc self-requested a review June 27, 2023 13:37
Copy link
Member

@esc esc 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. A few tiny nitpicks and one question:

does this need a documentation update?

block = self.block(name='my_block')
builder = ir.IRBuilder(block)
with self.assertRaises(AssertionError):
builder.comment("so\nmany\nlines")
Copy link
Member

Choose a reason for hiding this comment

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

Please use only a single \n here to ensure the test works with only a single new line.

builder = ir.IRBuilder(block)
with self.assertRaises(AssertionError):
builder.comment("so\nmany\nlines")
builder.comment("yo!")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
builder.comment("yo!")
builder.comment("my comment")

I would appreciate if you could use slightly less "street" style vernacular here.

builder.ret_void()
self.check_block(block, """\
my_block:
; yo!
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
; yo!
; my comment

@apmasell
Copy link
Contributor Author

What documentation would need updating?

@esc
Copy link
Member

esc commented Jul 11, 2023

What documentation would need updating?

The documentation for llvmlite isn't generated automatically, so I think this will need an entry for the comment function in:

https://llvmlite.readthedocs.io/en/latest/user-guide/ir/ir-builder.html#miscellaneous

@esc
Copy link
Member

esc commented Jul 11, 2023

I have made an issue to track that the llvmlite docs should be moved to use autodoc: #972

@esc
Copy link
Member

esc commented Jul 11, 2023

@apmasell this looks good. One last request: can you synchronise the docstring in the code-base and the text in the RST based documentation? I.e. such that they have the same verbatim text. That would save some time when we move to autodoc in future, because we will nuke all the "manually created" docs.

@apmasell
Copy link
Contributor Author

Okay. Synchronized.

@esc
Copy link
Member

esc commented Jul 11, 2023

Okay. Synchronized.

Great, waiting for CI and will then merge!

As titled.
@esc
Copy link
Member

esc commented Jul 11, 2023

@apmasell looks like flake8 is reporting some tabs and space mixing:

llvmlite/ir/builder.py:1050:1: W191 indentation contains tabs
llvmlite/ir/builder.py:1050:1: E101 indentation contains mixed spaces and tabs
llvmlite/ir/builder.py:1051:1: W191 indentation contains tabs
llvmlite/ir/builder.py:1051:1: E101 indentation contains mixed spaces and tabs
llvmlite/ir/builder.py:1052:1: W191 indentation contains tabs
llvmlite/ir/builder.py:1052:1: E101 indentation contains mixed spaces and tabs

@apmasell
Copy link
Contributor Author

Fixed.

@esc esc merged commit ff1d118 into numba:main Jul 11, 2023
@apmasell apmasell deleted the comment branch July 11, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow inserting comments using IRBuilder
2 participants