Skip to content

STYLE: Remove unused parameter from SetGlobalInstance and Singleton#4209

Merged
dzenanz merged 1 commit into
InsightSoftwareConsortium:masterfrom
N-Dekker:Remove-unused-parameter-SetGlobalInstance-Singleton
Sep 18, 2023
Merged

STYLE: Remove unused parameter from SetGlobalInstance and Singleton#4209
dzenanz merged 1 commit into
InsightSoftwareConsortium:masterfrom
N-Dekker:Remove-unused-parameter-SetGlobalInstance-Singleton

Conversation

@N-Dekker

@N-Dekker N-Dekker commented Sep 7, 2023

Copy link
Copy Markdown
Contributor

Added overloads of SetGlobalInstance and Singleton without the unused func parameter. Let the new SetGlobalInstance overload just return void, instead of bool. Deprecated the original overloads.

Follow-up to:

pull request #4164 commit 6d1c4c7
"STYLE: SingletonIndex does not need to store the unused func parameter"

pull request #4162 commit 6ec6328
"STYLE: Let Singleton assume that SetGlobalInstance always returns true"

@github-actions github-actions Bot added area:Core Issues affecting the Core module type:Style Style changes: no logic impact (indentation, comments, naming) labels Sep 7, 2023
@dzenanz

dzenanz commented Sep 7, 2023

Copy link
Copy Markdown
Member

We are probably going to do a major version bump soon, so this might be guarded under future legacy, instead of plain legacy.

@N-Dekker

N-Dekker commented Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

We are probably going to do a major version bump soon, so this might be guarded under future legacy, instead of plain legacy.

So then, would it still be OK to keep the proposed [[deprecated]] attributes?

@dzenanz

dzenanz commented Sep 7, 2023

Copy link
Copy Markdown
Member

Absolutely!

@N-Dekker
N-Dekker marked this pull request as ready for review September 7, 2023 14:43
@N-Dekker

N-Dekker commented Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

@dzenanz Can you do a Request changes to make it ITK_FUTURE_LEGACY_REMOVE? 😃

dzenanz
dzenanz reviewed Sep 8, 2023
View reviewed changes

@dzenanz dzenanz left a comment

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.

Like this?

Comment thread Modules/Core/Common/include/itkSingleton.h Outdated
Comment thread Modules/Core/Common/include/itkSingleton.h Outdated
dzenanz
dzenanz approved these changes Sep 8, 2023
View reviewed changes

@dzenanz dzenanz left a comment

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.

Squash upon merge?

@N-Dekker

N-Dekker commented Sep 8, 2023

Copy link
Copy Markdown
Contributor Author

@dzenanz GitHub automatically made you a co-author of the commits you suggested. Not sure if your co-authorship will be preserved when squashing. 😸 But I would like the commits to be squashed anyway 😄

@dzenanz

dzenanz commented Sep 8, 2023

Copy link
Copy Markdown
Member

Please completely disregard my co-authorship. I prefer a single commit too!

@thewtex
thewtex requested a review from blowekamp September 9, 2023 23:59
@N-Dekker
STYLE: Remove unused parameter from SetGlobalInstance and Singleton
eec0c42 
Added overloads of `SetGlobalInstance` and `Singleton` without the unused `func`
parameter. Let the new `SetGlobalInstance` overload just return `void`, instead
of `bool`. Deprecated the original overloads, and made them
`ITK_FUTURE_LEGACY_REMOVE`.

Follow-up to:

pull request InsightSoftwareConsortium#4164
commit 6d1c4c7
"STYLE: SingletonIndex does not need to store the unused `func` parameter"

pull request InsightSoftwareConsortium#4162
commit 6ec6328
"STYLE: Let `Singleton` assume that SetGlobalInstance always returns true"
@N-Dekker
N-Dekker force-pushed the Remove-unused-parameter-SetGlobalInstance-Singleton branch from f812c52 to eec0c42 Compare September 14, 2023 10:01
@N-Dekker

N-Dekker commented Sep 15, 2023

Copy link
Copy Markdown
Contributor Author

/azp run ITK.Linux.Python

@N-Dekker

N-Dekker commented Sep 18, 2023

Copy link
Copy Markdown
Contributor Author

I think the PR is ready now, but the CI says:

ghostflow-check-master Expected — Waiting for status to be reported

Any clue?

@dzenanz
dzenanz merged commit 8598841 into InsightSoftwareConsortium:master Sep 18, 2023
@dzenanz

dzenanz commented Sep 18, 2023

Copy link
Copy Markdown
Member

I overrode it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dzenanz dzenanz dzenanz approved these changes
@blowekamp blowekamp Awaiting requested review from blowekamp

Assignees

No one assigned

Labels

area:Core Issues affecting the Core module type:Style Style changes: no logic impact (indentation, comments, naming)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@N-Dekker @dzenanz