r240146 - Introduce type nullability specifiers for C/C++.

[Douglas Gregor]

> On Jun 19, 2015, at 11:10 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 
> On Fri, Jun 19, 2015 at 1:51 PM, Douglas Gregor <dgregor at apple.com <mailto:dgregor at apple.com>> wrote:
>> Author: dgregor
>> Date: Fri Jun 19 12:51:05 2015
>> New Revision: 240146
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=240146&view=rev
>> Log:
>> Introduce type nullability specifiers for C/C++.
>> 
>> Introduces the type specifiers __nonnull, __nullable, and
>> __null_unspecified that describe the nullability of the pointer type
>> to which the specifier appertains. Nullability type specifiers improve
>> on the existing nonnull attributes in a few ways:
>>  - They apply to types, so one can represent a pointer to a non-null
>>    pointer, use them in function pointer types, etc.
>>  - As type specifiers, they are syntactically more lightweight than
>>    __attribute__s or [[attribute]]s.
>>  - They can express both the notion of 'should never be null' and
>>  also 'it makes sense for this to be null', and therefore can more
>>  easily catch errors of omission where one forgot to annotate the
>>  nullability of a particular pointer (this will come in a subsequent
>>  patch).
>> 
>> Nullability type specifiers are maintained as type sugar, and
>> therefore have no effect on mangling, encoding, overloading,
>> etc. Nonetheless, they will be used for warnings about, e.g., passing
>> 'null' to a method that does not accept it.
>> 
>> This is the C/C++ part of rdar://problem/18868820.
> 
> This is great stuff! I have some trivial comments below.
> 
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=240146&r1=240145&r2=240146&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/Attr.td (original)
>> +++ cfe/trunk/include/clang/Basic/Attr.td Fri Jun 19 12:51:05 2015
>> @@ -960,6 +960,22 @@ def ReturnsNonNull : InheritableAttr {
>>   let Documentation = [Undocumented];
>> }
>> 
>> +// Nullability type attributes.
>> +def TypeNonNull : TypeAttr {
>> +  let Spellings = [Keyword<"__nonnull">];
>> +  let Documentation = [Undocumented];
>> +}
>> +
>> +def TypeNullable : TypeAttr {
>> +  let Spellings = [Keyword<"__nullable">];
>> +  let Documentation = [Undocumented];
>> +}
>> +
>> +def TypeNullUnspecified : TypeAttr {
>> +  let Spellings = [Keyword<"__null_unspecified">];
>> +  let Documentation = [Undocumented];
>> +}
> 
> Please add documentation for these type attributes (we frown on new
> attributes that are flagged as Undocumented).

Good call, r240296.

>> Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=240146&r1=240145&r2=240146&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Fri Jun 19 12:51:05 2015
>> @@ -65,6 +65,10 @@ def ext_keyword_as_ident : ExtWarn<
>>   "%select{here|for the remainder of the translation unit}1">,
>>   InGroup<KeywordCompat>;
>> 
>> +def ext_nullability : Extension<
>> +  "type nullability specifier %0 is a Clang extension">,
>> +  InGroup<DiagGroup<"nullability-extension">>;
> 
> Please have %0 quoted in the diagnostic.

It’s not necessary; we pass in an IdentifierInfo* here.

> 
>> 
>> Modified: cfe/trunk/lib/Parse/ParseDecl.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=240146&r1=240145&r2=240146&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Parse/ParseDecl.cpp (original)
>> +++ cfe/trunk/lib/Parse/ParseDecl.cpp Fri Jun 19 12:51:05 2015
>> @@ -687,6 +687,28 @@ void Parser::ParseOpenCLQualifiers(Parse
>>                AttributeList::AS_Keyword);
>> }
>> 
>> +void Parser::ParseNullabilityTypeSpecifiers(ParsedAttributes &attrs) {
>> +  // Treat these like attributes, even though they're type specifiers.
>> +  while (true) {
>> +    switch (Tok.getKind()) {
>> +    case tok::kw___nonnull:
>> +    case tok::kw___nullable:
>> +    case tok::kw___null_unspecified: {
>> +      IdentifierInfo *AttrName = Tok.getIdentifierInfo();
>> +      SourceLocation AttrNameLoc = ConsumeToken();
>> +      if (!getLangOpts().ObjC1)
>> +        Diag(AttrNameLoc, diag::ext_nullability)
>> +          << AttrName;
>> +      attrs.addNew(AttrName, AttrNameLoc, nullptr, AttrNameLoc, nullptr, 0,
>> +                   AttributeList::AS_Keyword);
>> +      break;
>> +    }
>> +    default:
>> +      return;
> 
> Would it be more clear to move the default to be the first label in
> the switch, and have it break instead of return (in case we would like
> to do post-processing of these at some point)?

Personally, I find it clearer the way it is.

>> 
>> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=240146&r1=240145&r2=240146&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Fri Jun 19 12:51:05 2015
>> @@ -2465,6 +2465,28 @@ static void mergeParamDeclAttributes(Par
>>   if (!foundAny) newDecl->dropAttrs();
>> }
>> 
>> +static void mergeParamDeclTypes(ParmVarDecl *NewParam,
>> +                                const ParmVarDecl *OldParam,
>> +                                Sema &S) {
>> +  if (auto Oldnullability = OldParam->getType()->getNullability(S.Context)) {
>> +    if (auto Newnullability = NewParam->getType()->getNullability(S.Context)) {
> 
> Can we put the dereference into the assignment expression, since these
> are only ever used as dereferenced?

We cannot, because we’re getting an Optional<NullabilityKind> here, and we need to check whether we have a value.

> 
>> +      if (*Oldnullability != *Newnullability) {
>> +        S.Diag(NewParam->getLocation(), diag::warn_mismatched_nullability_attr)
>> +          << static_cast<unsigned>(*Newnullability)
>> +          << static_cast<unsigned>(*Oldnullability);
>> +        S.Diag(OldParam->getLocation(), diag::note_previous_declaration);
>> +      }
>> +    }
>> +    else {
> 
> Formatting.

Okay.

>> +/// Distribute a nullability type attribute that cannot be applied to
>> +/// the type specifier to a pointer, block pointer, or member pointer
>> +/// declarator, complaining if necessary.
>> +///
>> +/// \returns true if the nullability annotation was distributed, false
>> +/// otherwise.
>> +static bool distributeNullabilityTypeAttr(TypeProcessingState &state,
>> +                                          QualType type,
>> +                                          AttributeList &attr) {
>> +  Declarator &declarator = state.getDeclarator();
>> +
>> +  /// Attempt to move the attribute to the specified chunk.
>> +  auto moveToChunk = [&](DeclaratorChunk &chunk, bool inFunction) -> bool {
>> +    // If there is already a nullability attribute there, don't add
>> +    // one.
>> +    if (hasNullabilityAttr(chunk.getAttrListRef()))
>> +      return false;
>> +
>> +    // Complain about the nullability qualifier being in the wrong
>> +    // place.
>> +    unsigned pointerKind
>> +      = chunk.Kind == DeclaratorChunk::Pointer ? (inFunction ? 3 : 0)
>> +        : chunk.Kind == DeclaratorChunk::BlockPointer ? 1
>> +        : inFunction? 4 : 2;
> 
> Can we replace the magic numbers with something else?

Sure, we can give these meaningful names.

	- Doug

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150622/035d1f9d/attachment.html>