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

stfpy: Added support to get header comments #42

Merged
merged 4 commits into from
Aug 8, 2024

Conversation

furuame
Copy link
Member

@furuame furuame commented Aug 1, 2024

Hello @bdutro,

We have a demand to get header comments from STFInstReader via stfpy. I dug into this and came up with a patch. However I’m not sure about my implementations as follows:

  • I need to get a vector of strings as type for header comments. However it was stored in STFReaderBase as std::vector<ConstHandle<CommentRecord>>. It was frustrating to me to implement the same interface in Cython. I kinda cheated it to add another private data member header_comments_str_ to save comments as strings when reading the header. But not sure if that’s the best way to implement it. Do you have thoughts on this?

  • I added a Cython class StringVector to accommodate the above data from STF C++ library. Not sure if I made(/wrote) it in the right convention/style. Let me know if it doesn’t work for you. Thanks!

@furuame
Copy link
Member Author

furuame commented Aug 5, 2024

I added a Cython class StringVector to accommodate the above data from STF C++ library. Not sure if I made(/wrote) it in the right convention/style. Let me know if it doesn’t work for you. Thanks!

I just renamed it to HeaderCommentsType. Should be fine now.

@bdutro
Copy link
Collaborator

bdutro commented Aug 5, 2024

I need to get a vector of strings as type for header comments. However it was stored in STFReaderBase as std::vector<ConstHandle>. It was frustrating to me to implement the same interface in Cython. I kinda cheated it to add another private data member header_comments_str_ to save comments as strings when reading the header. But not sure if that’s the best way to implement it. Do you have thoughts on this?

Since we generally don't need to read header comments, what if instead you only populated that vector if someone calls getHeaderCommentsString?

@furuame
Copy link
Member Author

furuame commented Aug 5, 2024

I need to get a vector of strings as type for header comments. However it was stored in STFReaderBase as std::vector. It was frustrating to me to implement the same interface in Cython. I kinda cheated it to add another private data member header_comments_str_ to save comments as strings when reading the header. But not sure if that’s the best way to implement it. Do you have thoughts on this?

Since we generally don't need to read header comments, what if instead you only populated that vector if someone calls getHeaderCommentsString?

That works for me! That’s how I did right now. So you’d suggest I could just remove the getHeaderComments method but keep getHeaderCommentsString?

@bdutro
Copy link
Collaborator

bdutro commented Aug 5, 2024

You can leave both methods. It doesn't hurt to give access to the underlying CommentRecords. What I'm saying is leave header_comments_str empty until someone calls getHeaderCommentsString() for the first time.

@furuame
Copy link
Member Author

furuame commented Aug 5, 2024

You can leave both methods. It doesn't hurt to give access to the underlying CommentRecords. What I'm saying is leave header_comments_str empty until someone calls getHeaderCommentsString() for the first time.

I see, will do!

@furuame
Copy link
Member Author

furuame commented Aug 5, 2024

You can leave both methods. It doesn't hurt to give access to the underlying CommentRecords. What I'm saying is leave header_comments_str empty until someone calls getHeaderCommentsString() for the first time.

Done!

Copy link
Collaborator

@bdutro bdutro left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bdutro bdutro merged commit 9bbe657 into sparcians:master Aug 8, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants