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

lcmgen: Emit file level comments #505

Merged
merged 5 commits into from
Sep 22, 2024
Merged

Conversation

judfs
Copy link
Contributor

@judfs judfs commented Apr 10, 2024

Parse any comment before the package statement in the .lcm file and apply it to the top of generated source files right after the 'generated with LCM' statement.

Does not apply if the package is defined via CLI flags.

Implemented for:

  • c
  • cpp
  • java
  • python
  • go
  • lua
  • c#

See test/types/lcmtest/comments_t.lcm and
build/test/types/lcmtest/comments_t.*

Parse any comment before the `package` statement in the .lcm file
and apply it to the top of generated source files right after the
'generated with LCM' statement.

Does not apply if the package is defined via CLI flags.

Implemented for:
- [x] c
- [x] cpp
- [x] java
- [x] python
- [ ] go
- [ ] lua
- [ ] c#

See test/types/lcmtest/comments_t.lcm and
build/test/types/lcmtest/comments_t.*
@judfs judfs marked this pull request as draft April 11, 2024 15:10
@judfs
Copy link
Contributor Author

judfs commented Apr 11, 2024

I just realized multiple types can be defined in 1 .lcm file. I did not account for that

@judfs
Copy link
Contributor Author

judfs commented May 7, 2024

https://lcm-proj.github.io/lcm/content/lcm-type-ref.html#type-specifications suggests (and also demands) one file per type. So should multiple types per file even be supported?

@judfs
Copy link
Contributor Author

judfs commented Aug 12, 2024

@ihilt any thoughts? I had put off implementing the other languages until I got feedback.

@ihilt
Copy link
Contributor

ihilt commented Aug 16, 2024

@ihilt any thoughts? I had put off implementing the other languages until I got feedback.

I'd stick with one type per file but I'm not tracking this super closely at the moment. @nosracd do you have any objections to that?

@nosracd
Copy link
Contributor

nosracd commented Aug 20, 2024

I'd stick with one type per file but I'm not tracking this super closely at the moment. @nosracd do you have any objections to that?

So to make sure I'm tracking:

The LCM documentation implies that there should be one struct per .lcm file, although it does not explicitly state it as a rule. The most strong wording I can find is this bit (talking about the struct temperature_t):

This declaration must appear in a file named temperature_t.lcm.

However, lcm-gen does support multiple structs per file and there's a chance that existing systems use it that way.

But, this PR will not affect any of the existing support for multiple structs per file. Instead, it's adding a new feature: file-level comments. And the current state of the PR is that it only supports file-level comments for .lcm files that only contain one struct per .lcm file. For example, if there are two structs defined in a .lcm file (foo_t first then bar_t) then only the output files for foo_t will have file-level comments.

Assuming I'm tracking all that correctly, then I think it's fine to only support file-level comments for .lcm files that have one struct per .lcm file. But I think the documentation should be more strongly worded to recommend that users only have one struct per .lcm file.

@judfs judfs marked this pull request as ready for review August 20, 2024 21:40
Copy link
Contributor

@nosracd nosracd 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. Noticed one minor typo but just let me know whenever you want me to hit the merge button

lcmgen/lcmgen.h Outdated Show resolved Hide resolved
fix typo

Co-authored-by: nosracd <djcdan24@gmail.com>
@judfs
Copy link
Contributor Author

judfs commented Sep 22, 2024

Looks like accepting the suggestion didn't cause formatting errors so should be good to go

@nosracd nosracd merged commit bdbda00 into lcm-proj:master Sep 22, 2024
49 checks passed
@judfs judfs deleted the file-level-comments branch September 23, 2024 18:16
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.

3 participants