[cfe-commits] r119583 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Basic/LangOptions.h include/clang/Driver/CC1Options.td include/clang/Driver/Options.td include/clang/Sema/Sema.h lib/Driver/Tools.cpp lib/Frontend/CompilerInvocation.cpp lib/Sema/SemaDecl.cpp test/SemaCXX/warn-argument-larger-than.cpp

[Chris Lattner]

On Nov 17, 2010, at 3:11 PM, Argyrios Kyrtzidis wrote:
> Author: akirtzidis
> Date: Wed Nov 17 17:11:54 2010
> New Revision: 119583
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=119583&view=rev
> Log:
> Introduce option -Wargument-larger-than[=N] which warns about function definitions if they take by-value
> or return by-value any POD that is larger than some threshold (default is 64 bytes).

Very nice, some minor suggestions:

> def CharSubscript : DiagGroup<"char-subscripts">;
> +def ArgumentSizeLargerThan : DiagGroup<"argument-larger-than">;

Since this applies to return values also, how about making this be -Wlarge-by-value-copy ?

> +def warn_parameter_size: Warning<"size of %0 is %1 bytes">,
> +  InGroup<ArgumentSizeLargerThan>;
> +def warn_return_value_size: Warning<"return value of %0 is %1 bytes">,
> +  InGroup<ArgumentSizeLargerThan>;

instead of just stating that it is N bytes, how about wording it like:

%0 is a large (%1 byte) pass-by-value argument; pass it by reference instead?

That makes it more clear what the suggested alternative is and why it's bad.

> 
> +void Sema::DiagnoseSizeOfParametersAndReturnValue(ParmVarDecl * const *Param,

Please add a doxygen comment explaining what this is doing and a couple comments in the code.

> +  if (ReturnTy->isPODType() &&
> +      Diags.getDiagnosticLevel(diag::warn_return_value_size) !=
> +          Diagnostic::Ignored) {

Is this worth doing a 'is disabled' check for it?  It doesn't seem that expensive.  Does it cause a lot of PCH deserialization or something?

-Chris