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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
