[cfe-commits] r148072 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/DeclSpec.h include/clang/Sema/Sema.h lib/Parse/ParseDecl.cpp lib/Parse/ParseDeclCXX.cpp lib/Parse/Parser.cpp lib/Sema/AnalysisBasedWarnings.cpp l

[Richard Smith]

On Fri, January 13, 2012 18:03, Matthieu Monrocq wrote:
> thank you very much for this, it looks really nice :)

Thanks!

> I only have two tiny remarks:
>
>> --- cfe/trunk/include/clang/Sema/Sema.h (original)
>> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Jan 12 17:53:29 2012
>> @@ -756,6 +756,9 @@
>>
>>
>> bool findMacroSpelling(SourceLocation &loc, StringRef name);
>>
>> +  /// \brief Get a string to suggest for zero-initialization of a type.
>> +  const char *getFixItZeroInitializerForType(QualType T) const;
>> +
>
> Why not use a StringRef here ?
>
> It is meant to be passed to FixItHint::CreateInsertion anyway, which will
> perform the conversion, and won't even benefit from __builtin_strlen to deduce
> the length.

Conversion to StringRef makes both call sites slightly uglier, because
StringRef doesn't have a contextual conversion to bool. More generally, it's
not clear to me whether StringRef is intended to semantically distinguish null
and non-null length-0 values. While the current implementation of this
function never returns a non-null empty string, previous drafts of it did, and
such a return value would mean something different from a null return.

The performance difference is interesting, but this function is only intended
to be used on very cold paths, so I don't find it compelling.

>> --- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)
>> +++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Thu Jan 12 17:53:29 2012
>>
>>
>> [...]
>>
>
>
>> +  SourceLocation Loc = S.PP.getLocForEndOfToken(VD->getLocEnd());
>> +  S.Diag(Loc, diag::note_var_fixit_add_initialization) <<
>> VD->getDeclName()
>> +    << FixItHint::CreateInsertion(Loc, Init);
>> +  return true;
>> }
>>
>>
>>
>
>> --- cfe/trunk/lib/Sema/SemaFixItUtils.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaFixItUtils.cpp Thu Jan 12 17:53:29 2012
>> @@ -158,3 +158,31 @@
>>
>>
>> return false; }
>> +
>> +const char *Sema::getFixItZeroInitializerForType(QualType T) const {
>> +  // Suggest 'nil' if it's defined and appropriate.
>> +  if ((T->isObjCObjectPointerType() || T->isBlockPointerType()) &&
>> +      PP.getMacroInfo(&getASTContext().Idents.get("nil")))
>> +    return " = nil";
>> +  if (T->isRealFloatingType())
>> +    return " = 0.0";
>> +  if (T->isBooleanType() && LangOpts.CPlusPlus)
>> +    return " = false";
>> +  if (T->isPointerType() || T->isMemberPointerType()) {
>> +    if (LangOpts.CPlusPlus0x)
>> +      return " = nullptr";
>> +    // Check if 'NULL' is defined.
>> +    else if (PP.getMacroInfo(&getASTContext().Idents.get("NULL")))
>> +      return " = NULL";
>>
>>
>
> Why not suggesting " = 0" here instead of letting the flow cascade ?

The same question applies to Obj-C pointers, block pointers and _Bool. The
intent of this arrangement was: if the type is a scalar type, produce " = 0"
unless we know of something better.

Does r148135 look better to you?

Thanks!
- Richard