This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add support for optional parameters in config.
ClosedPublic

Authored by felix642 on Sep 4 2023, 7:01 PM.

Details

Summary

If a parameter value is either 'none', 'null', 'false', '-1' or '', we will in that case use the default value

Diff Detail

Event Timeline

felix642 created this revision.Sep 4 2023, 7:01 PM
Herald added a project: Restricted Project. · View Herald Transcript
felix642 requested review of this revision.Sep 4 2023, 7:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2023, 7:01 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
felix642 updated this revision to Diff 555788.Sep 4 2023, 7:13 PM

Added entry to release notes

felix642 updated this revision to Diff 555789.Sep 4 2023, 7:15 PM

Reworded release notes

Thanks for the patch! I must admit I don't fully understand what problem it solves, i.e. parameters are already optional today (one just simply doesn't specify them in the config file). Why would we want to explicitly spell out parameters with some default value that doesn't correspond to what actually gets applied in the check?

carlosgalvezp added inline comments.Sep 5 2023, 11:14 AM
clang-tools-extra/test/clang-tidy/infrastructure/optional-parameter.cpp
18 ↗(On Diff #555789)

What if:

  • "MinimumLength" is a boolean.
  • It's default value (if not specified) is True.
  • And a user wants to set it as "False" here

Wouldn't that cause problems?

PiotrZSL added inline comments.Sep 5 2023, 11:32 AM
clang-tools-extra/clang-tidy/ClangTidyCheck.h
196–198

To be honest I wanted this to work only if we call get with Default being std::optional<T>, and such method should return optional, and store methods should be also updated.
And next time use github pull requests.

clang-tools-extra/test/clang-tidy/infrastructure/optional-parameter.cpp
18 ↗(On Diff #555789)

this functionality should work only for integers...
Idea is to have:

template <typename T>
std::enable_if_t<std::is_integral_v<T>, std::optional<T>>
getLocalOrGlobal(StringRef LocalName, std::optional<T> Default) const;

 template <typename T>
 std::enable_if_t<std::is_integral_v<T>, std::optional<T>>
 get(StringRef LocalName, std::optional<T> Default) const;

without impacting other functions... so that instead of doing tricks with -1 in code, we could simply create real optional options, and be able to 'store' them as optional, this mean that --dump-config should dump them as "none".

felix642 updated this revision to Diff 556326.Sep 8 2023, 5:18 PM

Updated Differential to truly support optional parameters

felix642 updated this revision to Diff 556328.Sep 8 2023, 5:25 PM

Updated documentation

felix642 updated this revision to Diff 556331.Sep 8 2023, 7:01 PM

Removed false option

felix642 updated this revision to Diff 556333.Sep 8 2023, 8:07 PM

Moved to a new method, changed tests, changed documentation

felix642 added a comment.EditedSep 8 2023, 8:11 PM

Hi @PiotrZSL and @carlosgalvezp, I have updated my diff based on your comments. Let me know what you think.

What if:

"MinimumLength" is a boolean.
It's default value (if not specified) is True.
And a user wants to set it as "False" here

Wouldn't that cause problems?

I have removed the "false" option to support optional for boolean types.

this functionality should work only for integers...
Idea is to have:

template <typename T>
std::enable_if_t<std::is_integral_v<T>, std::optional<T>>
getLocalOrGlobal(StringRef LocalName, std::optional<T> Default) const;

 template <typename T>
 std::enable_if_t<std::is_integral_v<T>, std::optional<T>>
 get(StringRef LocalName, std::optional<T> Default) const;

without impacting other functions... so that instead of doing tricks with -1 in code, we could simply create real optional options, and be able to 'store' them as optional, this mean that --dump-config should dump them as "none".

I have moved the declaration to a new method which works only on integral types where the default value is std::optional. Let me know what you think.

felix642 updated this revision to Diff 556334.Sep 8 2023, 8:12 PM

Forgot to update a comment

PiotrZSL accepted this revision.Sep 18 2023, 10:43 PM

Except few nits in documentation looks fine. Probably one day in future we can add same for bools.
Make sure that documentation of impacted checks is correct, so it says that argument is positive integer, or none to force default value.

clang-tools-extra/clang-tidy/readability/FunctionSizeCheck.h
20–34

Those options documentation could be removed from this file. Only keep check description.

clang-tools-extra/docs/ReleaseNotes.rst
116–118

I'm not sure if this release notes entry is needed. Maybe this change should be documented per changed check basic.

clang-tools-extra/docs/clang-tidy/checks/readability/function-size.rst
15

Leave previous but change -1 into none.

This revision is now accepted and ready to land.Sep 18 2023, 10:43 PM
felix642 updated this revision to Diff 557545.Oct 2 2023, 7:02 PM
felix642 marked 3 inline comments as done.

Updated documentation

felix642 updated this revision to Diff 557546.Oct 2 2023, 7:03 PM

Wrong check name

This revision was automatically updated to reflect the committed changes.