Comment Sema: Eliminate or factor out DeclInfo inspection (NFC)

[aaronpuchert]

Summary

We make all predicates expect an already inspected DeclInfo, and
introduce a function to run such a predicate after inspecting it.
Checks that were only used once have been inlined.

Before every call to inspectThisDecl the caller was checking IsFilled,
so we move that into the function itself.

[aaronpuchert]

clang/include/clang/AST/CommentSema.h

207

Another idea for naming: hasDeclThat, then calls read like hasDeclThat(isOfSomeKind).

[gribozavr2]

Sorry, here I also find the old code to be more readable.

• I don't see a problem with checks that are only used once. They are encapsulated in functions with meaningful names, making the code more readable. Compare Sema::checkFunctionDeclVerbatimLine before and after, for example.

• checkDecl looks like a too complex abstraction for the task at hand: it accepts a function pointer (and it is incorrect for the user to call those other functions directly), it hardcodes the result value of false when the decl is not available etc.

[aaronpuchert]

In D113795#3137250, @gribozavr2 wrote:

Sorry, here I also find the old code to be more readable.

Admittedly this isn't mainly about readability but about reducing boilerplate. A large number of functions start like this:

if (!ThisDeclInfo)
  return false;
if (!ThisDeclInfo->IsFilled)
  inspectThisDecl();

The diffstat is 95 insertions, 208 deletions.

• I don't see a problem with checks that are only used once. They are encapsulated in functions with meaningful names, making the code more readable. Compare Sema::checkFunctionDeclVerbatimLine before and after, for example.

Agreed, though you'll find I've basically only inlined one- or maybe two-liners. Having them in-place reduces the number of visual jumps in the code flow. Maybe isFunctionPointerVarDecl is worth preserving though.

• checkDecl looks like a too complex abstraction for the task at hand: it accepts a function pointer

Perhaps that's an issue with the name? I suggested hasDeclThat, then it's clear that this takes a predicate. Also reads almost like a proper sentence: hasDeclThat(isXXX).

(and it is incorrect for the user to call those other functions directly),

Not quite true, as you can see I'm calling many of these functions directly. We go through checkDecl only if we don't know if ThisDeclInfo is available.

it hardcodes the result value of false when the decl is not available etc.

Would hasDeclThat address that concern?

[aaronpuchert]

clang/lib/AST/CommentSema.cpp

135–137

Something might be wrong here anyway: the documentation says that @callback

Specifies the name and description of a callback field in a structure.

But we're looking for a VarDecl instead of a FieldDecl. Is somebody still around who knows HeaderDoc? Seems like Apple doesn't care any longer.

[aaronpuchert]

Rename checkDecl to hasDeclThat, which should hopefully address most issues.

We leave isFunctionPointerVarDecl inlined for now, since it's probably wrong anyway. (I think we should look for fields.)

clang/include/clang/AST/CommentSema.h

private:
   void checkContainerDeclVerbatimLine(const BlockCommandComment *Comment);
 
   void checkContainerDecl(const BlockCommandComment *Comment);
 
   /// Resolve parameter names to parameter indexes in function declaration.
   /// Emit diagnostics about unknown parametrs.
   void resolveParamCommandIndexes(const FullComment *FC);
 
-  /// \returns \c true if the declaration that this comment is attached to
-  /// is a pointer to function/method/block type or has such a type.
-  bool involvesFunctionType();
-
-  bool isFunctionDecl();
-  bool isAnyFunctionDecl();
-
-  /// \returns \c true if declaration that this comment is attached to declares
-  /// a function pointer.
-  bool isFunctionPointerVarDecl();
-  bool isFunctionOrMethodVariadic();
-  bool isObjCMethodDecl();
-  bool isObjCPropertyDecl();
-  bool isTemplateOrSpecialization();
-  bool isRecordLikeDecl();
-  bool isClassOrStructDecl();
-  /// \return \c true if the declaration that this comment is attached to
-  /// declares either struct, class or tag typedef.
-  bool isClassOrStructOrTagTypedefDecl();
-  bool isUnionDecl();
-  bool isObjCInterfaceDecl();
-  bool isObjCProtocolDecl();
-  bool isClassTemplateDecl();
-  bool isFunctionTemplateDecl();
+  bool hasDeclThat(bool (*check)(const DeclInfo &));
 
   ArrayRef<const ParmVarDecl *> getParamVars();
 
   /// Extract all important semantic information from
   /// \c ThisDeclInfo->ThisDecl into \c ThisDeclInfo members.
   void inspectThisDecl();
 
   /// Returns index of a function parameter with a given name.

clang/lib/AST/CommentSema.cpp

if (ThisDeclInfo) {
     // declaration.
     if (Info->IsReturnsCommand)
       checkReturnsCommand(Command);
     if (Info->IsDeprecatedCommand)
       checkDeprecatedCommand(Command);
   }
 }
 
+/// \returns \c true if the declaration that this comment is attached to
+/// is a pointer to function/method/block type or has such a type.
+static bool involvesFunctionType(const DeclInfo &Info) {
+  return Info.involvesFunctionType();
+}
+
 ParamCommandComment *Sema::actOnParamCommandStart(
                                       SourceLocation LocBegin,
                                       SourceLocation LocEnd,
                                       unsigned CommandID,
                                       CommandMarkerKind CommandMarker) {
   ParamCommandComment *Command =
       new (Allocator) ParamCommandComment(LocBegin, LocEnd, CommandID,
                                           CommandMarker);
 
-  if (!involvesFunctionType())
+  if (!hasDeclThat(involvesFunctionType))
     Diag(Command->getLocation(),
          diag::warn_doc_param_not_attached_to_a_function_decl)
       << CommandMarker
       << Command->getCommandNameRange(Traits);
 
   return Command;
 }
 
 void Sema::checkFunctionDeclVerbatimLine(const BlockCommandComment *Comment) {
+  if (ThisDeclInfo)
+    inspectThisDecl();
   unsigned DiagSelect;
   switch (Comment->getCommandID()) {
   case CommandTraits::KCI_function:
   case CommandTraits::KCI_functiongroup:
-    if (isAnyFunctionDecl() || isFunctionTemplateDecl())
+    if (ThisDeclInfo && ThisDeclInfo->getKind() == DeclInfo::FunctionKind &&
+        (isa<FunctionDecl>(ThisDeclInfo->CurrentDecl) ||
+         isa<FunctionTemplateDecl>(ThisDeclInfo->CurrentDecl)))
       return;
     DiagSelect = 0;
     break;
   case CommandTraits::KCI_method:
   case CommandTraits::KCI_methodgroup:
-    if (isObjCMethodDecl())
+    if (ThisDeclInfo && ThisDeclInfo->getKind() == DeclInfo::FunctionKind &&
+        isa<ObjCMethodDecl>(ThisDeclInfo->CurrentDecl))
       return;
     DiagSelect = 1;
     break;
   case CommandTraits::KCI_callback:
-    if (isFunctionPointerVarDecl())
+    if (ThisDeclInfo && ThisDeclInfo->getKind() == DeclInfo::VariableKind)
+      if (const auto *VD = dyn_cast<VarDecl>(ThisDeclInfo->CurrentDecl))
+        if (VD->getType()->isFunctionPointerType())
           return;
     DiagSelect = 2;
     break;
   default:
     llvm_unreachable("Unhandled command with IsFunctionDeclarationCommand");
   }
   Diag(Comment->getLocation(), diag::warn_doc_function_method_decl_mismatch)
       << Comment->getCommandMarker()
       << Traits.getCommandInfo(Comment->getCommandID())->Name << DiagSelect
       << Comment->getSourceRange();
 }
 
+static bool isClassOrStructDeclImpl(const Decl *D) {
+  if (auto *record = dyn_cast_or_null<RecordDecl>(D))
+    return !record->isUnion();
+
+  return false;
+}
+
+/// \return \c true if the declaration that this comment is attached to
+/// declares either struct, class or tag typedef.
+static bool isClassOrStructOrTagTypedefDecl(const DeclInfo &Info) {
+  if (!Info.CurrentDecl)
+    return false;
+
+  if (isClassOrStructDeclImpl(Info.CurrentDecl))
+    return true;
+
+  if (auto *ThisTypedefDecl = dyn_cast<TypedefDecl>(Info.CurrentDecl)) {
+    auto UnderlyingType = ThisTypedefDecl->getUnderlyingType();
+    if (auto ThisElaboratedType = dyn_cast<ElaboratedType>(UnderlyingType)) {
+      auto DesugaredType = ThisElaboratedType->desugar();
+      if (auto *DesugaredTypePtr = DesugaredType.getTypePtrOrNull()) {
+        if (auto *ThisRecordType = dyn_cast<RecordType>(DesugaredTypePtr)) {
+          return isClassOrStructDeclImpl(ThisRecordType->getAsRecordDecl());
+        }
+      }
+    }
+  }
+
+  return false;
+}
+
+static bool isObjCInterfaceDecl(const DeclInfo &Info) {
+  return isa_and_nonnull<ObjCInterfaceDecl>(Info.CurrentDecl);
+}
+
+static bool isObjCProtocolDecl(const DeclInfo &Info) {
+  return isa_and_nonnull<ObjCProtocolDecl>(Info.CurrentDecl);
+}
+
 void Sema::checkContainerDeclVerbatimLine(const BlockCommandComment *Comment) {
+  if (ThisDeclInfo) {
+    inspectThisDecl();
     switch (Comment->getCommandID()) {
     case CommandTraits::KCI_class:
-      if (isClassOrStructOrTagTypedefDecl() || isClassTemplateDecl())
+      if (isClassOrStructOrTagTypedefDecl(*ThisDeclInfo) ||
+          isa_and_nonnull<ClassTemplateDecl>(ThisDeclInfo->CurrentDecl))
         return;
       // Allow @class command on @interface declarations.
       // FIXME. Currently, \class and @class are indistinguishable. So,
       // \class is also allowed on an @interface declaration
-      if (Comment->getCommandMarker() && isObjCInterfaceDecl())
+      if (Comment->getCommandMarker() && isObjCInterfaceDecl(*ThisDeclInfo))
         return;
       break;
     case CommandTraits::KCI_interface:
-      if (isObjCInterfaceDecl())
+      if (isObjCInterfaceDecl(*ThisDeclInfo))
         return;
       break;
     case CommandTraits::KCI_protocol:
-      if (isObjCProtocolDecl())
+      if (isObjCProtocolDecl(*ThisDeclInfo))
         return;
       break;
     case CommandTraits::KCI_struct:
-      if (isClassOrStructOrTagTypedefDecl())
+      if (isClassOrStructOrTagTypedefDecl(*ThisDeclInfo))
         return;
       break;
     case CommandTraits::KCI_union:
-      if (isUnionDecl())
+      if (const RecordDecl *RD =
+              dyn_cast_or_null<RecordDecl>(ThisDeclInfo->CurrentDecl))
+        if (RD->isUnion())
           return;
       break;
     default:
       llvm_unreachable("Unhandled command with IsRecordLikeDeclarationCommand");
     }
+  }
   Diag(Comment->getLocation(), diag::warn_doc_api_container_decl_mismatch)
       << Comment->getCommandMarker()
       << Traits.getCommandInfo(Comment->getCommandID())->Name
       << Comment->getSourceRange();
 }
 
+static bool isRecordLikeDecl(const DeclInfo &Info) {
+  return isa_and_nonnull<RecordDecl>(Info.CurrentDecl) ||
+         isObjCInterfaceDecl(Info) || isObjCProtocolDecl(Info);
+}
+
 void Sema::checkContainerDecl(const BlockCommandComment *Comment) {
-  if (isRecordLikeDecl())
+  if (hasDeclThat(isRecordLikeDecl))
     return;
   Diag(Comment->getLocation(), diag::warn_doc_container_decl_mismatch)
       << Comment->getCommandMarker()
       << Traits.getCommandInfo(Comment->getCommandID())->Name
       << Comment->getSourceRange();
 }
 
 /// Turn a string into the corresponding PassDirection or -1 if it's not

 void Sema::actOnParamCommandFinish(ParamCommandComment *Command,
                                    ParagraphComment *Paragraph) {
   Command->setParagraph(Paragraph);
   assert(
       !Traits.getCommandInfo(Command->getCommandID())->IsEmptyParagraphAllowed);
   checkBlockCommandEmptyParagraph(Command);
 }
 
+static bool isTemplateOrSpecialization(const DeclInfo &Info) {
+  return Info.getTemplateKind() != DeclInfo::NotTemplate;
+}
+
 TParamCommandComment *Sema::actOnTParamCommandStart(
                                       SourceLocation LocBegin,
                                       SourceLocation LocEnd,
                                       unsigned CommandID,
                                       CommandMarkerKind CommandMarker) {
   TParamCommandComment *Command =
       new (Allocator) TParamCommandComment(LocBegin, LocEnd, CommandID,
                                            CommandMarker);
 
-  if (!isTemplateOrSpecialization())
+  if (!hasDeclThat(isTemplateOrSpecialization))
     Diag(Command->getLocation(),
          diag::warn_doc_tparam_not_attached_to_a_template_decl)
       << CommandMarker
       << Command->getCommandNameRange(Traits);
 
   return Command;
 }
 
 void Sema::actOnTParamCommandParamNameArg(TParamCommandComment *Command,
                                           SourceLocation ArgLocBegin,
                                           SourceLocation ArgLocEnd,
                                           StringRef Arg) {
   // Parser will not feed us more arguments than needed.
   assert(Command->getNumArgs() == 0);
 
   typedef BlockCommandComment::Argument Argument;
   Argument *A = new (Allocator) Argument(SourceRange(ArgLocBegin,
                                                      ArgLocEnd),
                                          Arg);
   Command->setArgs(llvm::makeArrayRef(A, 1));
 
-  if (!isTemplateOrSpecialization()) {
+  if (!hasDeclThat(isTemplateOrSpecialization)) {
     // We already warned that this \\tparam is not attached to a template decl.
     return;
   }
 
   const TemplateParameterList *TemplateParameters =
       ThisDeclInfo->TemplateParameters;
   SmallVector<unsigned, 2> Position;
   if (resolveTParamReference(Arg, TemplateParameters, &Position)) {
Diag(DiagLoc, diag::warn_doc_block_command_empty_paragraph)
       << Command->getCommandMarker()
       << Command->getCommandName(Traits)
       << Command->getSourceRange();
   }
 }
 
 void Sema::checkReturnsCommand(const BlockCommandComment *Command) {
   assert(ThisDeclInfo && "should not call this check on a bare comment");
+  inspectThisDecl();
 
   // We allow the return command for all @properties because it can be used
   // to document the value that the property getter returns.
-  if (isObjCPropertyDecl())
+  if (ThisDeclInfo->CurrentDecl->getKind() == Decl::ObjCProperty)
     return;
-  if (involvesFunctionType()) {
+  if (involvesFunctionType(*ThisDeclInfo)) {
     assert(!ThisDeclInfo->ReturnType.isNull() &&
            "should have a valid return type");
     if (ThisDeclInfo->ReturnType->isVoidType()) {
       unsigned DiagKind;
       switch (ThisDeclInfo->CommentDecl->getKind()) {
       default:
         if (ThisDeclInfo->IsObjCMethod)
           DiagKind = 3;
if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
     TextToInsert += " ";
     SourceLocation Loc = FD->getSourceRange().getBegin();
     Diag(Loc, diag::note_add_deprecation_attr)
         << FixItHint::CreateInsertion(Loc, TextToInsert);
   }
 }
 
 void Sema::resolveParamCommandIndexes(const FullComment *FC) {
-  if (!involvesFunctionType()) {
+  if (!hasDeclThat(involvesFunctionType)) {
     // We already warned that \\param commands are not attached to a function
     // decl.
     return;
   }
 
   SmallVector<ParamCommandComment *, 8> UnresolvedParamCommands;
 
   // Comment AST nodes that correspond to \c ParamVars for which we have
if (CorrectedParamIndex != ParamCommandComment::InvalidParamIndex) {
       if (const IdentifierInfo *CorrectedII = CorrectedPVD->getIdentifier())
         Diag(ArgRange.getBegin(), diag::note_doc_param_name_suggestion)
           << CorrectedII->getName()
           << FixItHint::CreateReplacement(ArgRange, CorrectedII->getName());
     }
   }
 }
 
-bool Sema::involvesFunctionType() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  return ThisDeclInfo->involvesFunctionType();
-}
-
-bool Sema::isFunctionDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  return ThisDeclInfo->getKind() == DeclInfo::FunctionKind;
-}
-
-bool Sema::isAnyFunctionDecl() {
-  return isFunctionDecl() && ThisDeclInfo->CurrentDecl &&
-         isa<FunctionDecl>(ThisDeclInfo->CurrentDecl);
-}
-
-bool Sema::isFunctionOrMethodVariadic() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  return ThisDeclInfo->IsVariadic;
-}
-
-bool Sema::isObjCMethodDecl() {
-  return isFunctionDecl() && ThisDeclInfo->CurrentDecl &&
-         isa<ObjCMethodDecl>(ThisDeclInfo->CurrentDecl);
-}
-
-bool Sema::isFunctionPointerVarDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  if (ThisDeclInfo->getKind() == DeclInfo::VariableKind) {
-    if (const VarDecl *VD = dyn_cast_or_null<VarDecl>(ThisDeclInfo->CurrentDecl)) {
-      QualType QT = VD->getType();
-      return QT->isFunctionPointerType();
-    }
-  }
-  return false;
-}
-
-bool Sema::isObjCPropertyDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  return ThisDeclInfo->CurrentDecl->getKind() == Decl::ObjCProperty;
-}
-
-bool Sema::isTemplateOrSpecialization() {
+bool Sema::hasDeclThat(bool (*check)(const DeclInfo &)) {
   if (!ThisDeclInfo)
     return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  return ThisDeclInfo->getTemplateKind() != DeclInfo::NotTemplate;
-}
-
-bool Sema::isRecordLikeDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  return isUnionDecl() || isClassOrStructDecl() || isObjCInterfaceDecl() ||
-         isObjCProtocolDecl();
-}
-
-bool Sema::isUnionDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  if (const RecordDecl *RD =
-        dyn_cast_or_null<RecordDecl>(ThisDeclInfo->CurrentDecl))
-    return RD->isUnion();
-  return false;
-}
-static bool isClassOrStructDeclImpl(const Decl *D) {
-  if (auto *record = dyn_cast_or_null<RecordDecl>(D))
-    return !record->isUnion();
-
-  return false;
-}
-
-bool Sema::isClassOrStructDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-
-  if (!ThisDeclInfo->CurrentDecl)
-    return false;
-
-  return isClassOrStructDeclImpl(ThisDeclInfo->CurrentDecl);
-}
-
-bool Sema::isClassOrStructOrTagTypedefDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-
-  if (!ThisDeclInfo->CurrentDecl)
-    return false;
-
-  if (isClassOrStructDeclImpl(ThisDeclInfo->CurrentDecl))
-    return true;
-
-  if (auto *ThisTypedefDecl = dyn_cast<TypedefDecl>(ThisDeclInfo->CurrentDecl)) {
-    auto UnderlyingType = ThisTypedefDecl->getUnderlyingType();
-    if (auto ThisElaboratedType = dyn_cast<ElaboratedType>(UnderlyingType)) {
-      auto DesugaredType = ThisElaboratedType->desugar();
-      if (auto *DesugaredTypePtr = DesugaredType.getTypePtrOrNull()) {
-        if (auto *ThisRecordType = dyn_cast<RecordType>(DesugaredTypePtr)) {
-          return isClassOrStructDeclImpl(ThisRecordType->getAsRecordDecl());
-        }
-      }
-    }
-  }
-
-  return false;
-}
-
-bool Sema::isClassTemplateDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  return ThisDeclInfo->CurrentDecl &&
-          (isa<ClassTemplateDecl>(ThisDeclInfo->CurrentDecl));
-}
-
-bool Sema::isFunctionTemplateDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  return ThisDeclInfo->CurrentDecl &&
-         (isa<FunctionTemplateDecl>(ThisDeclInfo->CurrentDecl));
-}
-
-bool Sema::isObjCInterfaceDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  return ThisDeclInfo->CurrentDecl &&
-         isa<ObjCInterfaceDecl>(ThisDeclInfo->CurrentDecl);
-}
-
-bool Sema::isObjCProtocolDecl() {
-  if (!ThisDeclInfo)
-    return false;
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
-  return ThisDeclInfo->CurrentDecl &&
-         isa<ObjCProtocolDecl>(ThisDeclInfo->CurrentDecl);
+  return check(*ThisDeclInfo);
 }
 
 ArrayRef<const ParmVarDecl *> Sema::getParamVars() {
-  if (!ThisDeclInfo->IsFilled)
-    inspectThisDecl();
   return ThisDeclInfo->ParamVars;
 }
 
 void Sema::inspectThisDecl() {
+  if (!ThisDeclInfo->IsFilled)
     ThisDeclInfo->fill();
 }
 
 unsigned Sema::resolveParmVarReference(StringRef Name,
                                        ArrayRef<const ParmVarDecl *> ParamVars) {
   for (unsigned i = 0, e = ParamVars.size(); i != e; ++i) {
     const IdentifierInfo *II = ParamVars[i]->getIdentifier();
     if (II && II->getName() == Name)
       return i;
   }
-  if (Name == "..." && isFunctionOrMethodVariadic())
+  if (Name == "..." && ThisDeclInfo->IsVariadic)
     return ParamCommandComment::VarArgParamIndex;
   return ParamCommandComment::InvalidParamIndex;
 }
 
 namespace {
 class SimpleTypoCorrector {
   const NamedDecl *BestDecl;