Le 13 janvier 2012 20:39, Richard Smith <rich...@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
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
