If a parameter value is either 'none', 'null', 'false', '-1' or '', we will in that case use the default value
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
clang-tools-extra/test/clang-tidy/infrastructure/optional-parameter.cpp | ||
---|---|---|
19 | What if:
Wouldn't that cause problems? |
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. | |
clang-tools-extra/test/clang-tidy/infrastructure/optional-parameter.cpp | ||
19 | this functionality should work only for integers... 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". |
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.
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 ↗ | (On Diff #556334) | 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 ↗ | (On Diff #556334) | Leave previous but change -1 into none. |
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.