FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-101282: Group and quote BOLT CLI flags by erlend-aasland · Pull Request #104821 · python/cpython · GitHub

/ cpython Public

gh-101282: Group and quote BOLT CLI flags - #104821

Closed
erlend-aasland wants to merge 1 commit into
python:mainfrom
erlend-aasland:fixup-bolt-config
ConversationCommits (1)ChecksFiles changed
Closed

gh-101282: Group and quote BOLT CLI flags#104821
erlend-aasland wants to merge 1 commit into
python:mainfrom
erlend-aasland:fixup-bolt-config

Conversation

erlend-aasland commented May 23, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

erlend-aasland requested a review from corona10 as a code owner May 23, 2023 21:27
bedevere-bot mentioned this pull request May 23, 2023
Closed
4 tasks
corona10 reviewed May 23, 2023
View reviewed changes

corona10 left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I am not sure why you want to separate the BOLT options.
Please don't do this, let's maintain options as easy possible.
#104752 is not enough?

corona10 commented May 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

When I talked about grouping the BOLT options, it doesn't mean separating them as variables.
It was about sorting.

corona10 reviewed May 23, 2023
View reviewed changes
Comment thread configure.ac
[-use-gnu-stack],
[-frame-opt=hot]
)]
["${BOLT_GENERIC_OPTIONS} ${BOLT_OPTIMIZATION_OPTIONS}"]

corona10 May 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

One pain point of this change is we have to care double quote in multiple places.
(defining BOLT_GENERIC_OPTIONS, defining BOLT_OPTIMIZATION_OPTIONS and BOLT_GENERIC_OPTIONS + BOLT_OPTIMIZATION_OPTIONS)

erlend-aasland May 24, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I disagree; I don't see that as a "pain point".

corona10 May 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I disagree; I don't see that as a "pain point".

Well, My preference is don't care about double quotes as possible.
(This was the root cause of this issue in my guess)
I feel quite uncomfortable maintaining three elements that should be noted by double quotes.
It could be my personal preference. but yeah that's why I said pain point.

corona10 requested review from Yhg1s and ned-deily May 23, 2023 22:59

erlend-aasland commented May 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I am not sure why you want to separate the BOLT options.
Please don't do this, let's maintain options as easy possible.

Currently, we only apply BOLT options to the optimisation stage. What if we want to add options during the instrumentation stage as well? In that case, won't it make sense to apply the generic flags to both stages? If so, will it not be easier to set the generic flags just once? Correct me if I'm wrong.

corona10 commented May 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

What if we want to add options during the instrumentation stage as well?


I am not sure at this moment, but my mind is reducing the unnecessary option as possible.
(If my hypothesis is correct, only the binary relocation option will remain)

erlend-aasland commented May 24, 2023

Copy link
Copy Markdown
Contributor Author

Closing this, as grouping is no longer needed.

erlend-aasland deleted the fixup-bolt-config branch May 24, 2023 07:01
erlend-aasland mentioned this pull request May 24, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

corona10 corona10 left review comments
Yhg1s Awaiting requested review from Yhg1s
ned-deily Awaiting requested review from ned-deily

Assignees

No one assigned

Labels

awaiting core review needs backport to 3.12 only security fixes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL