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

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

Closed
wants to merge 1 commit into from

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented May 23, 2023

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

corona10 commented May 23, 2023

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

[-use-gnu-stack],
[-frame-opt=hot]
)]
["${BOLT_GENERIC_OPTIONS} ${BOLT_OPTIMIZATION_OPTIONS}"]
Copy link
Member

@corona10 corona10 May 23, 2023

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 corona10 requested review from Yhg1s and ned-deily May 23, 2023 22:59
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented May 24, 2023

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
Copy link
Member

corona10 commented May 24, 2023

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
Copy link
Contributor Author

Closing this, as grouping is no longer needed.

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.

3 participants