modernize-use-auto

[Jacobfaib]

Description

closes

Checklist

• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[fbusato]

I'm generally in favor of that, but my feeling is that @gevtushenko and @miscco could have a different opinion

[Jacobfaib]

I'm generally in favor of that, but my feeling is that @gevtushenko and @miscco could have a different opinion

To perhaps allay some fears here, clang-tidy will only suggest using auto when the type is already named on the same line:

• Casts: Foo f = static_cast<Foo>(x) -> auto f = static_cast<Foo>(x)
• Construction: Foo f = Foo{...}; -> auto f = Foo{...};
• New expressions: Foo *f = new Foo{...}; -> auto *f = new Foo{...};

Or specifically for standard library iterators:

std::vector<int>::iterator it = v.begin();
// becomes
auto it = v.begin();

[bernhardmgruber]

This PR is like 95% fine and I prefer the refactoring. I looked through a third or so.

[bernhardmgruber]

Remark: haven't seen a reinterpretation as a reference to an array in a long time

[bernhardmgruber]

This looks a bit like clang-tidy knows that PortionOffsetT is int. Can we reflect this in the cast?

Suggested change

	auto max_num_blocks = ::cuda::ceil_div(
	static_cast<int>(::cuda::std::min(num_items, static_cast<OffsetT>(PORTION_SIZE))), ONESWEEP_TILE_ITEMS);
	auto max_num_blocks = ::cuda::ceil_div(
	static_cast<PortionOffsetT>(::cuda::std::min(num_items, static_cast<OffsetT>(PORTION_SIZE))), ONESWEEP_TILE_ITEMS);

[Jacobfaib]

Hmmm I am confused, the int cast already existed before, you want to change that to PortionOffsetT as well?

[github-actions]

😬 CI Workflow Results

🟥 Finished in 3h 17m: Pass: 99%/379 | Total: 9d 08h | Max: 2h 07m | Hits: 89%/528882

See results here.

[miscco]

I am really not sure about this. Auto is often usefull if we have a static_cast on the other side of an assignment

But in many cases this creates visual noise, when we have multiple variables and one of them suddenly becomes auto

Often its also not easy to keep the type in min for more complex code, so I am not sure whether its a win

[miscco]

I am against this one, it breaks alignnment

[Jacobfaib]

What if I made them all auto? And

const SampleT lower_level_r = 0;
// to
const auto lower_level_r = SamepleT{0};

I can silence them with // NOLINT but I'd rather avoid that if there are other code changes we can make to satisfy the linter.

[fbusato]

we could enforce variable alignment 😁 https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignconsecutivedeclarations

[miscco]

Those also break alignment

[miscco]

Ditto breaks alignment

[Jacobfaib]

Since there seems to be no concerted votes for this feature, I will go ahead and close this PR and leave the check disabled.