Skip to content

STYLE: Let MemoryProbesCollecterBaseTest use std::this_thread::sleep_for - #7110

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
N-Dekker:MemoryProbesCollecterBaseTest-use-std-this_thread-sleep_for
Nov 4, 2026
Merged

hjmjohnson merged 1 commit into
InsightSoftwareConsortium:mainfrom
N-Dekker:MemoryProbesCollecterBaseTest-use-std-this_thread-sleep_for

Conversation

@N-Dekker

@N-Dekker N-Dekker commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Replaced the use of platform specific sleep functions. Replaced a global inline function and a Sleep macro definition with a more modern lambda. Removed the milliseconds parameter, as it was always 5000 anyway.

@github-actions github-actions Bot added type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module type:Style Style changes: no logic impact (indentation, comments, naming) labels Nov 4, 2026

@hjmjohnson hjmjohnson 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.

Love the use of platform independant standards!

@N-Dekker
N-Dekker marked this pull request as ready for review November 4, 2026 15:40
@greptile-apps

greptile-apps Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The PR appears safe to merge.

Summary

Replaces platform-specific sleep code in itkMemoryProbesCollecterBaseTest.cxx with a local lambda using std::this_thread::sleep_for.

  • The memory-probe test uses one optional five-second wait helper.

Reviews (2) · Last reviewed commit: "STYLE: Let MemoryProbesCollecterBaseTest..." · Reviewed by Greptile

itkMemoryProbesCollecterBaseTest(int, char *[])
{
constexpr auto sleepIfTestWithSleep = [] {
#if defined(TEST_WITH_SLEEP)

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.

Instead of the #if defined(TEST_WITH_SLEEP), we could also introduce a static constexpr local bool variable testWithSleep, and then do if constexpr (testWithSleep) here, to ensure that the syntax of the sleep_for call is always checked, unconditionally. Just if anyone likes that better... 😃

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.

It seems to me that in practice, the macro TEST_WITH_SLEEP is never defined! If I'm correct, that would mean that the proposed std::this_thread::sleep_for call would never be evaluated by the compiler. So then a syntax error or a missing #include would not be detected for the std::this_thread::sleep_for call. So maybe we should do something like if constexpr (testWithSleep) here, instead of #if defined(TEST_WITH_SLEEP)!

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.

The ROI on the alternative approach is low. My personal opinion is that it would not be enough of an improvement to justify the extra work. I'm Fine if you choose to do it, but I would rather put the effort into other, more impactful PR reviews.

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.

Thanks but in general I think we should prefer if constexpr over #if, for the sake of maintainability. This just aims to be a good example. AI agents might learn from our examples 😉

Hereby force-pushed!

@N-Dekker
N-Dekker marked this pull request as draft November 4, 2026 16:07
Replaced the use of platform specific sleep functions. Replaced a global `inline` function and a `Sleep` macro definition with a more modern lambda. Removed the `milliseconds` parameter, as it was always `5000` anyway.
@N-Dekker
N-Dekker force-pushed the MemoryProbesCollecterBaseTest-use-std-this_thread-sleep_for branch from 8f775ea to 8b83276 Compare November 4, 2026 16:15
@N-Dekker
N-Dekker marked this pull request as ready for review November 4, 2026 16:43
@hjmjohnson
hjmjohnson merged commit 10e0d8d into InsightSoftwareConsortium:main Nov 4, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Style Style changes: no logic impact (indentation, comments, naming) type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants