Improve code base using clang-tidy

[AndrewPaxie]

Trompeloeil may be further improved through the use of clang-tidy and its library of checks.

To start this process, I am conducting a brief survey of Trompeloeil and clang-tidy users to gauge the number of warnings clang-tidy is reporting in include/trompeloeil.hpp.

This discussion will help inform the strategy for resolving in-production clang-tidy warnings that may be distracting attention from production code issues.

One example issue may be found in #139, line 4334: misc-non-private-member-variables-in-classes.

Suggested comment response format:

• Version of Trompeloeil used (e.g. v34 or master).
• Version(s) of clang-tidy used.
• Command line used to invoke clang-tidy.
• Configuration file (if any) used with clang-tidy.
• (Optional) Ranked list of issues reported.

Responses are requested by 31 May 2019, but planning is likely to be undertaken during the Request for Comments (RFC) period.

[reddwarf69]

• Trompeloeil v34 (plus two of the three commits master has over v34, they are mine :-p )
• clang-tidy 8.0.0
• /usr/share/clang/run-clang-tidy.py -header-filter '.*'

---
Checks:          '*,-fuchsia-default-arguments,-fuchsia-overloaded-operator,-fuchsia-trailing-return,-llvm-header-guard,-google-runtime-references,-clang-diagnostic-*,-readability-magic-numbers'
WarningsAsErrors: '*'
CheckOptions:
  - key:             readability-identifier-naming.ClassCase
    value:           lower_case
  - key:             readability-identifier-naming.ConstantCase
    value:           lower_case
  - key:             readability-identifier-naming.EnumCase
    value:           lower_case
  - key:             readability-identifier-naming.FunctionCase
    value:           lower_case
  - key:             readability-identifier-naming.MemberCase
    value:           lower_case
  - key:             readability-identifier-naming.NamespaceCase
    value:           lower_case
  - key:             readability-identifier-naming.ParameterCase
    value:           lower_case
  - key:             readability-identifier-naming.TemplateParameterCase
    value:           CamelCase
  - key:             readability-identifier-naming.TypeAliasCase
    value:           lower_case
  - key:             readability-identifier-naming.TypedefCase
    value:           lower_case
  - key:             readability-identifier-naming.UnionCase
    value:           lower_case
  - key:             readability-identifier-naming.VariableCase
    value:           lower_case
  - key:             readability-identifier-naming.MemberSuffix
    value:           _
...

The only single warning I get is the one from https://github.com/rollbear/trompeloeil/pull/139

Notice that you can't ever guarantee it will be clang-tidy warning-free. If the user sets readability-identifier-naming options there is always going to be warnings for some specific naming style. I happen to be lucky in using the same style than trompeloeil. Even forcing the use of snack_case in trompeloeil_movable_mock would be a problem for those heretics using camelCase.

Warnings in trompeloeil.hpp are not an issue to me, I mark the directory where trompeloeil.hpp is as a "system" include directory since I'm not interested in the warnings it produces. clang-tidy honours this the same as the compiler. My only issue is when a macro from trompeloeil.hpp, expanded in my own source code, produces something that makes clang-tidy complain. In the case of https://github.com/rollbear/trompeloeil/pull/139 I have got that warning when expanding TROMPELOEIL_MAKE_MOCK<x>.

[AndrewPaxie]

Trompeloeil version: master (@048f24e61ec52aad3f978dabc54b3f19a17ec855) Clang version: clang-latest (@41338e902f5e68dd856327f67baab8d4d8088cd1) Command line:

# Create compile_commands.json with clang++-latest
$ cd build/

$ CXX="clang++-latest" \
cmake \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
-DCMAKE_BUILD_TYPE=Debug \
..

$ cd ..

# Run clang-tidy
$ /opt/llvm-latest/bin/clang-tidy \
-p build/compile_commands.json \
test/compiling_tests_14.cpp &> /tmp/first-run.txt

File .clang-tidy

---
# Checks:          'clang-diagnostic-*,clang-analyzer-*,*'
Checks:          'clang-diagnostic-*,clang-analyzer-*,*,-cppcoreguidelines-macro-usage,-fuchsia-default-arguments,-fuchsia-overloaded-operator,-google-runtime-int,-llvm-header-guard,-llvm-include-order'

# WarningsAsErrors: ''
# HeaderFilterRegex: ''
HeaderFilterRegex: '.*'
AnalyzeTemporaryDtors: false
# FormatStyle:     none
User:            apaxie
CheckOptions:
  - key:             abseil-string-find-startswith.AbseilStringsMatchHeader
    value:           'absl/strings/match.h'
  - key:             abseil-string-find-startswith.IncludeStyle
    value:           llvm
  - key:             abseil-string-find-startswith.StringLikeClasses
    value:           '::std::basic_string'
  - key:             bugprone-argument-comment.StrictMode
    value:           '0'
  - key:             bugprone-assert-side-effect.AssertMacros
    value:           assert
  - key:             bugprone-assert-side-effect.CheckFunctionCalls
    value:           '0'
  - key:             bugprone-dangling-handle.HandleClasses
    value:           'std::basic_string_view;std::experimental::basic_string_view'
  - key:             bugprone-exception-escape.FunctionsThatShouldNotThrow
    value:           ''
  - key:             bugprone-exception-escape.IgnoredExceptions
    value:           ''
  - key:             bugprone-misplaced-widening-cast.CheckImplicitCasts
    value:           '0'
  - key:             bugprone-sizeof-expression.WarnOnSizeOfCompareToConstant
    value:           '1'
  - key:             bugprone-sizeof-expression.WarnOnSizeOfConstant
    value:           '1'
  - key:             bugprone-sizeof-expression.WarnOnSizeOfIntegerExpression
    value:           '0'
  - key:             bugprone-sizeof-expression.WarnOnSizeOfThis
    value:           '1'
  - key:             bugprone-string-constructor.LargeLengthThreshold
    value:           '8388608'
  - key:             bugprone-string-constructor.WarnOnLargeLength
    value:           '1'
  - key:             bugprone-suspicious-enum-usage.StrictMode
    value:           '0'
  - key:             bugprone-suspicious-missing-comma.MaxConcatenatedTokens
    value:           '5'
  - key:             bugprone-suspicious-missing-comma.RatioThreshold
    value:           '0.200000'
  - key:             bugprone-suspicious-missing-comma.SizeThreshold
    value:           '5'
  - key:             bugprone-suspicious-string-compare.StringCompareLikeFunctions
    value:           ''
  - key:             bugprone-suspicious-string-compare.WarnOnImplicitComparison
    value:           '1'
  - key:             bugprone-suspicious-string-compare.WarnOnLogicalNotComparison
    value:           '0'
  - key:             bugprone-unused-return-value.CheckedFunctions
    value:           '::std::async;::std::launder;::std::remove;::std::remove_if;::std::unique;::std::unique_ptr::release;::std::basic_string::empty;::std::vector::empty'
  - key:             cert-dcl16-c.NewSuffixes
    value:           'L;LL;LU;LLU'
  - key:             cert-dcl59-cpp.HeaderFileExtensions
    value:           ',h,hh,hpp,hxx'
  - key:             cert-err09-cpp.CheckThrowTemporaries
    value:           '1'
  - key:             cert-err61-cpp.CheckThrowTemporaries
    value:           '1'
  - key:             cert-msc32-c.DisallowedSeedTypes
    value:           'time_t,std::time_t'
  - key:             cert-msc51-cpp.DisallowedSeedTypes
    value:           'time_t,std::time_t'
  - key:             cert-oop11-cpp.IncludeStyle
    value:           llvm
  - key:             cppcoreguidelines-avoid-magic-numbers.IgnoredFloatingPointValues
    value:           '1.0;100.0;'
  - key:             cppcoreguidelines-avoid-magic-numbers.IgnoredIntegerValues
    value:           '1;2;3;4;'
  - key:             cppcoreguidelines-macro-usage.AllowedRegexp
    value:           '^DEBUG_*'
  - key:             cppcoreguidelines-macro-usage.CheckCapsOnly
    value:           '0'
  - key:             cppcoreguidelines-macro-usage.IgnoreCommandLineMacros
    value:           '1'
  - key:             cppcoreguidelines-no-malloc.Allocations
    value:           '::malloc;::calloc'
  - key:             cppcoreguidelines-no-malloc.Deallocations
    value:           '::free'
  - key:             cppcoreguidelines-no-malloc.Reallocations
    value:           '::realloc'
  - key:             cppcoreguidelines-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic
    value:           '1'
  - key:             cppcoreguidelines-owning-memory.LegacyResourceConsumers
    value:           '::free;::realloc;::freopen;::fclose'
  - key:             cppcoreguidelines-owning-memory.LegacyResourceProducers
    value:           '::malloc;::aligned_alloc;::realloc;::calloc;::fopen;::freopen;::tmpfile'
  - key:             cppcoreguidelines-pro-bounds-constant-array-index.GslHeader
    value:           ''
  - key:             cppcoreguidelines-pro-bounds-constant-array-index.IncludeStyle
    value:           '0'
  - key:             cppcoreguidelines-pro-type-member-init.IgnoreArrays
    value:           '0'
  - key:             cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions
    value:           '0'
  - key:             cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor
    value:           '0'
  - key:             fuchsia-header-anon-namespaces.HeaderFileExtensions
    value:           ',h,hh,hpp,hxx'
  - key:             fuchsia-restrict-system-includes.Includes
    value:           '*'
  - key:             google-build-namespaces.HeaderFileExtensions
    value:           ',h,hh,hpp,hxx'
  - key:             google-global-names-in-headers.HeaderFileExtensions
    value:           ',h,hh,hpp,hxx'
  - key:             google-readability-braces-around-statements.ShortStatementLines
    value:           '1'
  - key:             google-readability-function-size.BranchThreshold
    value:           '4294967295'
  - key:             google-readability-function-size.LineThreshold
    value:           '4294967295'
  - key:             google-readability-function-size.NestingThreshold
    value:           '4294967295'
  - key:             google-readability-function-size.ParameterThreshold
    value:           '4294967295'
  - key:             google-readability-function-size.StatementThreshold
    value:           '800'
  - key:             google-readability-function-size.VariableThreshold
    value:           '4294967295'
  - key:             google-readability-namespace-comments.ShortNamespaceLines
    value:           '10'
  - key:             google-readability-namespace-comments.SpacesBeforeComments
    value:           '2'
  - key:             google-runtime-int.SignedTypePrefix
    value:           int
  - key:             google-runtime-int.TypeSuffix
    value:           ''
  - key:             google-runtime-int.UnsignedTypePrefix
    value:           uint
  - key:             google-runtime-references.WhiteListTypes
    value:           ''
  - key:             hicpp-braces-around-statements.ShortStatementLines
    value:           '0'
  - key:             hicpp-function-size.BranchThreshold
    value:           '4294967295'
  - key:             hicpp-function-size.LineThreshold
    value:           '4294967295'
  - key:             hicpp-function-size.NestingThreshold
    value:           '4294967295'
  - key:             hicpp-function-size.ParameterThreshold
    value:           '4294967295'
  - key:             hicpp-function-size.StatementThreshold
    value:           '800'
  - key:             hicpp-function-size.VariableThreshold
    value:           '4294967295'
  - key:             hicpp-member-init.IgnoreArrays
    value:           '0'
  - key:             hicpp-move-const-arg.CheckTriviallyCopyableMove
    value:           '1'
  - key:             hicpp-multiway-paths-covered.WarnOnMissingElse
    value:           '0'
  - key:             hicpp-named-parameter.IgnoreFailedSplit
    value:           '0'
  - key:             hicpp-no-malloc.Allocations
    value:           '::malloc;::calloc'
  - key:             hicpp-no-malloc.Deallocations
    value:           '::free'
  - key:             hicpp-no-malloc.Reallocations
    value:           '::realloc'
  - key:             hicpp-special-member-functions.AllowMissingMoveFunctions
    value:           '0'
  - key:             hicpp-special-member-functions.AllowSoleDefaultDtor
    value:           '0'
  - key:             hicpp-uppercase-literal-suffix.NewSuffixes
    value:           ''
  - key:             hicpp-use-auto.MinTypeNameLength
    value:           '5'
  - key:             hicpp-use-auto.RemoveStars
    value:           '0'
  - key:             hicpp-use-emplace.ContainersWithPushBack
    value:           '::std::vector;::std::list;::std::deque'
  - key:             hicpp-use-emplace.SmartPointers
    value:           '::std::shared_ptr;::std::unique_ptr;::std::auto_ptr;::std::weak_ptr'
  - key:             hicpp-use-emplace.TupleMakeFunctions
    value:           '::std::make_pair;::std::make_tuple'
  - key:             hicpp-use-emplace.TupleTypes
    value:           '::std::pair;::std::tuple'
  - key:             hicpp-use-equals-default.IgnoreMacros
    value:           '1'
  - key:             hicpp-use-equals-delete.IgnoreMacros
    value:           '1'
  - key:             hicpp-use-noexcept.ReplacementString
    value:           ''
  - key:             hicpp-use-noexcept.UseNoexceptFalse
    value:           '1'
  - key:             hicpp-use-nullptr.NullMacros
    value:           ''
  - key:             llvm-namespace-comment.ShortNamespaceLines
    value:           '1'
  - key:             llvm-namespace-comment.SpacesBeforeComments
    value:           '1'
  - key:             misc-definitions-in-headers.HeaderFileExtensions
    value:           ',h,hh,hpp,hxx'
  - key:             misc-definitions-in-headers.UseHeaderFileExtension
    value:           '1'
  - key:             misc-throw-by-value-catch-by-reference.CheckThrowTemporaries
    value:           '1'
  - key:             misc-unused-parameters.StrictMode
    value:           '0'
  - key:             modernize-loop-convert.MaxCopySize
    value:           '16'
  - key:             modernize-loop-convert.MinConfidence
    value:           reasonable
  - key:             modernize-loop-convert.NamingStyle
    value:           CamelCase
  - key:             modernize-make-shared.IgnoreMacros
    value:           '1'
  - key:             modernize-make-shared.IncludeStyle
    value:           '0'
  - key:             modernize-make-shared.MakeSmartPtrFunction
    value:           'std::make_shared'
  - key:             modernize-make-shared.MakeSmartPtrFunctionHeader
    value:           memory
  - key:             modernize-make-unique.IgnoreMacros
    value:           '1'
  - key:             modernize-make-unique.IncludeStyle
    value:           '0'
  - key:             modernize-make-unique.MakeSmartPtrFunction
    value:           'std::make_unique'
  - key:             modernize-make-unique.MakeSmartPtrFunctionHeader
    value:           memory
  - key:             modernize-pass-by-value.IncludeStyle
    value:           llvm
  - key:             modernize-pass-by-value.ValuesOnly
    value:           '0'
  - key:             modernize-raw-string-literal.ReplaceShorterLiterals
    value:           '0'
  - key:             modernize-replace-auto-ptr.IncludeStyle
    value:           llvm
  - key:             modernize-replace-random-shuffle.IncludeStyle
    value:           llvm
  - key:             modernize-use-auto.MinTypeNameLength
    value:           '5'
  - key:             modernize-use-auto.RemoveStars
    value:           '0'
  - key:             modernize-use-default-member-init.IgnoreMacros
    value:           '1'
  - key:             modernize-use-default-member-init.UseAssignment
    value:           '0'
  - key:             modernize-use-emplace.ContainersWithPushBack
    value:           '::std::vector;::std::list;::std::deque'
  - key:             modernize-use-emplace.SmartPointers
    value:           '::std::shared_ptr;::std::unique_ptr;::std::auto_ptr;::std::weak_ptr'
  - key:             modernize-use-emplace.TupleMakeFunctions
    value:           '::std::make_pair;::std::make_tuple'
  - key:             modernize-use-emplace.TupleTypes
    value:           '::std::pair;::std::tuple'
  - key:             modernize-use-equals-default.IgnoreMacros
    value:           '1'
  - key:             modernize-use-equals-delete.IgnoreMacros
    value:           '1'
  - key:             modernize-use-noexcept.ReplacementString
    value:           ''
  - key:             modernize-use-noexcept.UseNoexceptFalse
    value:           '1'
  - key:             modernize-use-nullptr.NullMacros
    value:           'NULL'
  - key:             modernize-use-transparent-functors.SafeMode
    value:           '0'
  - key:             modernize-use-using.IgnoreMacros
    value:           '1'
  - key:             objc-forbidden-subclassing.ForbiddenSuperClassNames
    value:           'ABNewPersonViewController;ABPeoplePickerNavigationController;ABPersonViewController;ABUnknownPersonViewController;NSHashTable;NSMapTable;NSPointerArray;NSPointerFunctions;NSTimer;UIActionSheet;UIAlertView;UIImagePickerController;UITextInputMode;UIWebView'
  - key:             objc-property-declaration.Acronyms
    value:           ''
  - key:             objc-property-declaration.IncludeDefaultAcronyms
    value:           '1'
  - key:             performance-faster-string-find.StringLikeClasses
    value:           'std::basic_string'
  - key:             performance-for-range-copy.AllowedTypes
    value:           ''
  - key:             performance-for-range-copy.WarnOnAllAutoCopies
    value:           '0'
  - key:             performance-inefficient-string-concatenation.StrictMode
    value:           '0'
  - key:             performance-inefficient-vector-operation.VectorLikeClasses
    value:           '::std::vector'
  - key:             performance-move-const-arg.CheckTriviallyCopyableMove
    value:           '1'
  - key:             performance-move-constructor-init.IncludeStyle
    value:           llvm
  - key:             performance-type-promotion-in-math-fn.IncludeStyle
    value:           llvm
  - key:             performance-unnecessary-copy-initialization.AllowedTypes
    value:           ''
  - key:             performance-unnecessary-value-param.AllowedTypes
    value:           ''
  - key:             performance-unnecessary-value-param.IncludeStyle
    value:           llvm
  - key:             portability-simd-intrinsics.Std
    value:           ''
  - key:             portability-simd-intrinsics.Suggest
    value:           '0'
  - key:             readability-braces-around-statements.ShortStatementLines
    value:           '0'
  - key:             readability-function-size.BranchThreshold
    value:           '4294967295'
  - key:             readability-function-size.LineThreshold
    value:           '4294967295'
  - key:             readability-function-size.NestingThreshold
    value:           '4294967295'
  - key:             readability-function-size.ParameterThreshold
    value:           '4294967295'
  - key:             readability-function-size.StatementThreshold
    value:           '800'
  - key:             readability-function-size.VariableThreshold
    value:           '4294967295'
  - key:             readability-identifier-naming.IgnoreFailedSplit
    value:           '0'
  - key:             readability-implicit-bool-conversion.AllowIntegerConditions
    value:           '0'
  - key:             readability-implicit-bool-conversion.AllowPointerConditions
    value:           '0'
  - key:             readability-inconsistent-declaration-parameter-name.IgnoreMacros
    value:           '1'
  - key:             readability-inconsistent-declaration-parameter-name.Strict
    value:           '0'
  - key:             readability-magic-numbers.IgnoredFloatingPointValues
    value:           '1.0;100.0;'
  - key:             readability-magic-numbers.IgnoredIntegerValues
    value:           '1;2;3;4;'
  - key:             readability-redundant-smartptr-get.IgnoreMacros
    value:           '1'
  - key:             readability-simplify-boolean-expr.ChainedConditionalAssignment
    value:           '0'
  - key:             readability-simplify-boolean-expr.ChainedConditionalReturn
    value:           '0'
  - key:             readability-simplify-subscript-expr.Types
    value:           '::std::basic_string;::std::basic_string_view;::std::vector;::std::array'
  - key:             readability-static-accessed-through-instance.NameSpecifierNestingThreshold
    value:           '3'
  - key:             readability-uppercase-literal-suffix.NewSuffixes
    value:           ''
  - key:             zircon-temporary-objects.Names
    value:           ''
...

Extracted warnings

Running

grep "trompeloeil.hpp.*warning\:" /tmp/first-run.txt > /tmp/trompeloeil-warning-list.txt
grep "\[.*\]" /tmp/trompeloeil-warning-list.txt | cut -d\[ -f 2 | cut -d\] -f 1 | sort -u

yields this list of clang-tidy warnings

bugprone-exception-escape
bugprone-forwarding-reference-overload
bugprone-macro-parentheses
bugprone-unhandled-self-assignment
cert-dcl21-cpp
cert-dcl50-cpp
clang-analyzer-optin.cplusplus.VirtualCall
cppcoreguidelines-avoid-c-arrays
cppcoreguidelines-avoid-magic-numbers
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
cppcoreguidelines-pro-bounds-pointer-arithmetic
cppcoreguidelines-pro-type-const-cast
cppcoreguidelines-pro-type-reinterpret-cast
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-special-member-functions
fuchsia-multiple-inheritance
fuchsia-trailing-return
google-explicit-constructor
google-runtime-operator
google-runtime-references
hicpp-braces-around-statements
hicpp-explicit-conversions
hicpp-no-array-decay
hicpp-noexcept-move
hicpp-signed-bitwise
hicpp-special-member-functions
hicpp-use-equals-delete
hicpp-use-override
hicpp-vararg
llvm-namespace-comment
misc-non-private-member-variables-in-classes
misc-unconventional-assign-operator
modernize-avoid-c-arrays
modernize-use-default-member-init
modernize-use-override
modernize-use-trailing-return-type
performance-noexcept-move-constructor
readability-braces-around-statements
readability-const-return-type
readability-implicit-bool-conversion
readability-magic-numbers
readability-named-parameter

Some of these warnings can be immediately suppressed, e.g. fuchsia-trailing-return and readability-magic-numbers.

There is a workaround for the short term, ignore these warnings with either -isystem on the compiler command line, or use -header-filter= when running clang-tidy.

Fixing the warning in #139 that bleeds through to application code via macro expansion seems a reasonable starting point.

[rollbear]

For some reason, it never occurred to me that there's an obvious difference between warnings that are about library internals, and things that leak into user code via macros. Very valuable observation there. Thanks.

One thing that keeps popping into my mind about suppressing warnings is Marshal Clow's C++Now presentation from 2017:

https://www.youtube.com/watch?v=65IbEUX3QZM

As in life in general, one has to choose which fights are worth fighting. I guess that what this issue boils down to is making that choice (damned!) User observable is stupendously high on that list.

[reddwarf69]

Latest master. This code

$ cat trompeloeil_test.cpp 
#include "trompeloeil.hpp"
#include <functional>

struct to_mock
{
#if 1
    virtual void func1(std::function<void(int)>) = 0;
#else
    virtual void func1(int) = 0;
#endif
};

struct my_mock : public trompeloeil::mock_interface<to_mock>
{
    IMPLEMENT_MOCK1(func1);
};
$

run under clang 8.0.0 with

$ clang-tidy -checks=-*,performance-unnecessary-value-param trompeloeil_test.cpp 
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "trompeloeil_test.cpp"
No compilation database found in /test or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
2 warnings generated.
/test/trompeloeil_test.cpp:15:5: error: the parameter 'p1' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
    IMPLEMENT_MOCK1(func1);
    ^
/test/trompeloeil.hpp:4831:35: note: expanded from macro 'IMPLEMENT_MOCK1'
#define IMPLEMENT_MOCK1           TROMPELOEIL_IMPLEMENT_MOCK1
                                  ^
/test/trompeloeil.hpp:4162:3: note: expanded from macro 'TROMPELOEIL_IMPLEMENT_MOCK1'
  TROMPELOEIL_IMPLEMENT_MOCK_(1, name)
  ^
/test/trompeloeil.hpp:4226:3: note: expanded from macro 'TROMPELOEIL_IMPLEMENT_MOCK_'
  TROMPELOEIL_MAKE_MOCK_(name,,num, decltype(::trompeloeil::nonconst_member_signature(&trompeloeil_interface_name::name))::type,override,)
  ^
note: (skipping 3 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
/test/trompeloeil.hpp:141:35: note: expanded from macro 'TROMPELOEIL_CONCAT_'
#define TROMPELOEIL_CONCAT_(x, y) x ## y
                                  ^
note: expanded from here
/test/trompeloeil.hpp:252:45: note: expanded from macro 'TROMPELOEIL_PARAM_LIST1'
  ::trompeloeil::param_list_t<func_type, 0> p1
                                            ^
1 warning treated as error
$

[rollbear]

That is because clang-tidy correctly identifies the signature of func() to take the std::function<> parameter by value. Unfortunately it's not clever enough to give a very helpful message, though. If you change your to_mock struct to:

struct to_mock
{
#if 1
    virtual void func1(const std::function<void(int)>&) = 0;
#else
    virtual void func1(int) = 0;
#endif
};

clang-tidy has no complaints.

[reddwarf69]

Are you saying there is something wrong with to_mock and it should be changed anyway? The semantics of the real "func1" I'm trying to mock are to take a callable object to store it. It's a "when X event occurs tell me calling this callback". Isn't the correct, efficient, thing to do to pass the std:: function by value?

[rollbear]

I'm saying that to_mock::func1() takes a std::function<> by value i.e. it is copied, and that is what clang-tidy warns about. "wrong" is debatable, but there is nothing that can be done in Trompeloeil about it. It can only generate code that matches the signature it is given.

[reddwarf69]

So you think in this case

--- trompeloeil.hpp
+++ trompeloeil.hpp
@@ -254,7 +254,7 @@
 #define TROMPELOEIL_PARAM_LIST0(func_type)

 #define TROMPELOEIL_PARAM_LIST(num, func_type)                                 \
-  TROMPELOEIL_CONCAT(TROMPELOEIL_PARAM_LIST, num)(func_type)
+  TROMPELOEIL_CONCAT(TROMPELOEIL_PARAM_LIST, num)(func_type) /* NOLINT(performance-unnecessary-value-param) */

 #define TROMPELOEIL_PARAMS15 TROMPELOEIL_PARAMS14, p15

would be justified?

[rollbear]

I think you may have a reason to file a bug report with clang-tidy. Look here:

#include <functional>

struct S
{
  virtual void func(std::function<void(int)>) = 0;
};

struct SV : S
{
  void func(std::function<void(int)>) override {};
};

struct SS
{
  void func(std::function<void(int)>) {};
};

Clang tidy gives the warning for SS::func(), but not for SV::func() or, for that matter, S::func(). I think it should warn already at S::func(), because the interface forces copying of the parameter, just as the warning for SS::func() says. Or is there some actual reason to not warn when the function is virtual? It doesn't make sense to me.

[reddwarf69]

The error message says the parameter 'p1' is copied for each invocation but only ***used*** as a const reference; consider making it a const reference. It's not complaining just because it's being passed by value, it's complaining because its usage doesn't require it being passed by value.

This doesn't generate the warning:

#include <functional>

struct S
{
    virtual void func(std::function<void(int)>) = 0;
};

struct SV : S
{
    void func(std::function<void(int)>) override{};
};

struct SS
{
    void func(std::function<void(int)> func) { func_ = std::move(func); };

    std::function<void(int)> func_;
};

since SS::func() has a good reason to get it by value.

It can't say anything about S::func() because it doesn't know how it will be used.

Not really sure why it doesn't complain about SV::func(). The documentation doesn't seem to say anything about it (https://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html). But it does seem to be because of the inheritance, and it seems to makes sense to me: "yes sure, in this specific derived class usage it would be more efficient to pass it by const reference... but it's not up to me, I have to do whatever the interface mandates. Supposedly the interface is as it is because in the common derived class usage passing by value is the best option".

[reddwarf69]

FWIW this is the expanded form of my original sample:

    ::trompeloeil::return_of_t<
            decltype(::trompeloeil::nonconst_member_signature(&trompeloeil_interface_name::func1))::type>
    func1(::trompeloeil::param_list_t<
            decltype(::trompeloeil::nonconst_member_signature(&trompeloeil_interface_name::func1))::type,
            0> p1) override
    {
        using T_func1 = typename std::remove_reference<decltype(*this)>::type;
        using pmf_s_t = typename ::trompeloeil::signature_to_member_function<
                T_func1,
                decltype(*this),
                decltype(::trompeloeil::nonconst_member_signature(
                        &trompeloeil_interface_name::func1))::type>::type const;
        using pmf_e_t = typename ::trompeloeil::signature_to_member_function<
                T_func1,
                trompeloeil_l_tag_type_trompeloeil_15,
                decltype(::trompeloeil::nonconst_member_signature(
                        &trompeloeil_interface_name::func1))::type>::type const;
        pmf_s_t const s_ptr = &T_func1::trompeloeil_self_func1;
        pmf_e_t const e_ptr = &T_func1::trompeloeil_tag_func1;
        ::trompeloeil::ignore(s_ptr, e_ptr);
        return ::trompeloeil::mock_func<trompeloeil_movable_mock,
                                        decltype(::trompeloeil::nonconst_member_signature(
                                                &trompeloeil_interface_name::func1))::type>(
                trompeloeil_l_cardinality_match_15{},
                trompeloeil_l_expectations_15,
                "func1",
                "decltype(::trompeloeil::nonconst_member_signature(&trompeloeil_interface_name::func1))::type",
                p1);
    }
    auto trompeloeil_self_func1(::trompeloeil::param_list_t<decltype(::trompeloeil::nonconst_member_signature(
                                                                    &trompeloeil_interface_name::func1))::type,
                                                            0> p1) -> decltype(*this)
    {
        ::trompeloeil::ignore("func1", p1);
        return *this;
    }
    trompeloeil_l_tag_type_trompeloeil_15 trompeloeil_tag_func1(
            ::trompeloeil::param_list_t<
                    decltype(::trompeloeil::nonconst_member_signature(&trompeloeil_interface_name::func1))::type,
                    0> p1)
    {
        ::trompeloeil::ignore("func1", p1);
        return { nullptr, 0ul, nullptr };
    }

clang-tidy is complaining about trompeloeil_self_func1 and trompeloeil_tag_func1, which only use p1 to pass it to ::trompeloeil::ignore. It's not really complaining about the func1 override at all.

This patch is already enough to remove the clang-tidy warnings:

--- trompeloeil.hpp
+++ trompeloeil.hpp
@@ -4311,7 +4311,7 @@
   }                                                                            \
                                                                                \
   auto                                                                         \
-  trompeloeil_self_ ## name(TROMPELOEIL_PARAM_LIST(num, sig)) constness        \
+  trompeloeil_self_ ## name(TROMPELOEIL_PARAM_LIST(num, sig)) constness  /* NOLINT(performance-unnecessary-value-param) */      \
     -> decltype(*this)                                                         \
   {                                                                            \
     ::trompeloeil::ignore(#name TROMPELOEIL_PARAMS(num));                      \
@@ -4319,7 +4319,7 @@
   }                                                                            \
                                                                                \
   TROMPELOEIL_LINE_ID(tag_type_trompeloeil)                                    \
-  trompeloeil_tag_ ## name(TROMPELOEIL_PARAM_LIST(num, sig)) constness         \
+  trompeloeil_tag_ ## name(TROMPELOEIL_PARAM_LIST(num, sig)) constness /* NOLINT(performance-unnecessary-value-param) */        \
   {                                                                            \
     ::trompeloeil::ignore(#name TROMPELOEIL_PARAMS(num));                      \
     return {nullptr, 0ul, nullptr};                                            \

[rollbear]

The sad thing is that these functions don't have to be implemented at all. They're only ever used on decltype() context to disambiguate overloads, so only the signature is needed. But if they're not implemented and referenced (although never actually called) clang complains about them. So the complaint is in a workaround to silence a warning. Adding a work around to a work around is so depressing. It's doubly depressing because these work arounds are liabilities in the sense that every time I see them, I cannot help but wonder if they're correct, or if they're hiding important information. That this uncertainty about code quality comes from trying to please tools, whose very reason to exist are to increase ones confidence in code quality, is nuts.

I'll try to figure out some other way of doing this. Adding another work around is madness (although I fear madness may be the only route available.)

[reddwarf69]

FWIW I do find these tools useful. They do find real improvement situations and automate stuff that would otherwise create discussions in PRs that I would rather avoid. I don't like having to include tool/compiler-specific annotations either. But I draw a line about workarounds if they make something more complex than it needs to be.

When I find such an issue I do try to write it in a different way to please the tool, I do w̶a̶s̶t̶e̶ spend time pleasing tools. Sometimes I end up with solutions that are actually better than the original. Most of the time they are neither better or worse, just different. But I refuse to commit anything that makes the code worse (by any metric of worse, including "a little bit more difficult to understand") just to please the tool. In such a case I just use the annotation.

doctest says it "Doesn't produce any warnings even on the most aggressive warning levels for MSVC/GCC/Clang". But it does so by using a nice set of helpers to just make the compilers shut-up ( https://github.com/onqtam/doctest/blob/2ce8c59c108b58c4f5c723c017eaad7b8fa80a4a/doctest/doctest.h#L92).

[rollbear]

I generally agree, but suppressing a diagnostic is by definition hiding a bug. The question is if its a bug in your code or in the tool emitting the diagnostic, and it is never ever a code quality improvement.

However, you may want to have a look at #145. Silencing the clang warning feels less harmful than suppressing a diagnostic caused by trying to get rid of the warning.

[reddwarf69]

clang-tidy 9.0.0 introduces a funny one: https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-trailing-return-type.html

Every TROMPELOEIL_MAKE_MOCK<X> macro triggers it :-( I'm still trying to decide whether to keep that check in my clang-tidy or not...

I have also created https://bugs.llvm.org/show_bug.cgi?id=43980.

[rollbear]

This one's harmless and won't clutter code. I actually quite like trailing return type. I think the reason Trompeloeil isn't written with it is just that I hadn't quite gotten used to the idea at the time.

But you're raising the interesting question about which diagnostics to bother with, and which ones to ignore. Trying to satisfy everything leads to madness (like clang's warning about C++11/14/17/2a code not being compatible with C++98, and if you stoop down to writing C++98 code, you get warnings about not using nullptr and other "modern" constructs.)

+1 for the llvm report. I'm guessing it'll be ignored, but it's good to raise the issue.