[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

[Argyrios Kyrtzidis]

On Nov 17, 2010, at 3:35 PM, Chris Lattner wrote:

> 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 ?

Sounds good.

> 
>> +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.

Ditto.

> 
>> 
>> +void Sema::DiagnoseSizeOfParametersAndReturnValue(ParmVarDecl * const *Param,
> 
> Please add a doxygen comment explaining what this is doing and a couple comments in the code.

There's a doxygen comment in the declaration, are we supposed to copy the comment in the definition as well ?
Seems a bit better to have it in just one place for easier updating though I'm not sure if it's better for doxygen to have it in definition too.

> 
>> +  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?

This is to allow disabling the warning through #pragma diagnostic.

-Argiris

> 
> -Chris