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

[workspace] Make vendor_cxx smarter #17334

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jun 5, 2022

Reduce the flapping of open/close namespaces.
No need to wrap blank lines nor preprocessor directives.

Required by #17230. Towards #17231.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: medium release notes: none This pull request should not be mentioned in the release notes labels Jun 5, 2022
@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review June 6, 2022 02:38
@jwnimmer-tri
Copy link
Collaborator Author

+@sammy-tri for feature review, please?

Copy link
Contributor

@sammy-tri sammy-tri left a comment

Choose a reason for hiding this comment

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

:lgtm: +@ggould-tri for platform review (rotation)

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri)

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:; nits.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions (waiting on @jwnimmer-tri)


tools/workspace/vendor_cxx.py line 80 at r1 (raw file):

                flags[i] = Flag.DONT_CARE
            else:
                flags[i] = Flag.WRAP

minor: It is unsound to mark trailing matter here as WRAP; it would be better to error.
For example, as written this generates invalid c++ on the following input:

#include <mumble>

/* some commentary
*/ class fumble {

};

(Also this case is untested)
(Also also, in any case I assert that trailing matter like this cannot be soundly resolved in any line-based parse, as I can always bridge WRAP and NO_WRAP matter with a C-style comment)


tools/workspace/vendor_cxx.py line 125 at r1 (raw file):

                    break

    # Anything remaining is DONT_CARE buffeted by WRAP, so we'll WRAP it.

nit: Is "buffeted" the right word here? This usage is unfamiliar to me. "Buffered" perhaps? "Enclosed"? "Surrounded"? "Defiled"?

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion (waiting on @ggould-tri)


tools/workspace/vendor_cxx.py line 80 at r1 (raw file):

Previously, ggould-tri wrote…

minor: It is unsound to mark trailing matter here as WRAP; it would be better to error.
For example, as written this generates invalid c++ on the following input:

#include <mumble>

/* some commentary
*/ class fumble {

};

(Also this case is untested)
(Also also, in any case I assert that trailing matter like this cannot be soundly resolved in any line-based parse, as I can always bridge WRAP and NO_WRAP matter with a C-style comment)

Test added, and code fixed to treat the entire block with a uniform flag.

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),sammy-tri(platform) (waiting on @jwnimmer-tri)


tools/workspace/vendor_cxx.py line 80 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Test added, and code fixed to treat the entire block with a uniform flag.

I am not seeing another revision here. Did you miss a push?

Reduce the flapping of open/close namespaces.
No need to wrap blank lines nor preprocessor directives.
@jwnimmer-tri jwnimmer-tri force-pushed the vendor-cxx-optimizations branch from c92bfe6 to e3fbe30 Compare June 7, 2022 13:42
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion (waiting on @ggould-tri and @sammy-tri)


tools/workspace/vendor_cxx.py line 80 at r1 (raw file):

Previously, ggould-tri wrote…

I am not seeing another revision here. Did you miss a push?

Indeed! Pushed now.

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

I expect I could still break this with some stupid code (A C comment that starts on an include line and ends on a class def line?) , and I'll write some up if you care, but I can't think of an even slightly reasonable case to break it with for now. lgtm

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),sammy-tri(platform) (waiting on @jwnimmer-tri)

@jwnimmer-tri
Copy link
Collaborator Author

I'm OK to defer for now. My basic premise here is that the unit test should cover the features of the tool, so that we can modify the tool's implementation with confidence.

It's very plausible that the tool's current feature set will still fail on various new externals (or upgrades to externals), most likely due to C++ language features (e.g., forward declarations, or "extern C") moreso than parsing mistakes. As long as the compilation fails when the tool fails, we will be fine -- we'll fail-fast. Then we can revisit the tool to adapt it to the particular of that failure, rather than Gedanken-bugs.

@jwnimmer-tri jwnimmer-tri merged commit d95656f into RobotLocomotion:master Jun 7, 2022
@jwnimmer-tri jwnimmer-tri deleted the vendor-cxx-optimizations branch June 7, 2022 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants