[cfe-commits] r137980 - in /cfe/trunk: include/clang/Basic/Builtins.def include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaChecking.cpp test/Sema/warn-strlcpycat-size.c

[Ted Kremenek]

On Aug 18, 2011, at 2:33 PM, Eli Friedman wrote:

>> +  // Output a FIXIT hint if the destination is an array (rather than a
>> +  // pointer to an array).  This could be enhanced to handle some
>> +  // pointers if we know the actual size, like if DstArg is 'array+2'
>> +  // we could say 'sizeof(array)-2'.
>> +  const Expr *DstArg = Call->getArg(0)->IgnoreParenImpCasts();
>> +
>> +  if (DstArg->getType()->isArrayType()) {
>> +    llvm::SmallString<128> sizeString;
>> +    llvm::raw_svector_ostream OS(sizeString);
>> +    OS << "sizeof(";
>> +    DstArg->printPretty(OS, Context, 0, Context.PrintingPolicy);
>> +    OS << ")";
>> +
>> +    Diag(OriginalSizeArg->getLocStart(), diag::note_strlcpycat_wrong_size)
>> +      << FixItHint::CreateReplacement(OriginalSizeArg->getSourceRange(),
>> +                                      OS.str());
>> +  }
>> +}
> 
> I would say this fixit isn't a good idea, both because we should be
> forcing the programmer to think about what they are doing here, and
> because it isn't necessarily correct.  (sizeof(dest) isn't guaranteed
> to be the actual size of the destination; suppose, for example, dest
> is a flexible array.)

If the array is of constant size, or even a VLA, I don't see any problem with emitting a FIXIT.  You're right that a FIXIT shouldn't be emitted for flexible arrays.

As for whether or not we should issue a FIXIT, I think we do more harm than good if we can emit a FIXIT that is 99% likely to be okay.  The problem we are trying to solve is that people use these APIs incorrectly all the time.  Where we are absolutely confident that we can correct the problem, I don't see the harm in doing so.  When you say "think about what they are doing here", the problem is that often the programmer doesn't know what they should be doing.  Sometimes showing them how to fix the problem goes a long way to educating them.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110818/a0a27174/attachment.html>