[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

[Matthieu Monrocq]

Le 13 janvier 2012 20:39, Richard Smith <richard at metafoo.co.uk> a écrit :

> 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.
>
>
Ah indeed, I didn't consider the null/empty distinction and I am not sure
whether StringRef is intended to cope with nullity.


> 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?
>
>
Yes, thank you.


> Thanks!
> - Richard
>
>
Thanks!
-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120114/7de790a8/attachment.html>