C++ ApiView: Add Support for the [[nodiscard]] attribute; clean up so…

…me ApiView diffs. (#7973) * Added nodiscard support; Exclude line numbers from diffs * Exclude friend declarations in the std namespace and in the _detail namespace * Update pool --------- Co-authored-by: Daniel Jurek <[email protected]>
Azure · Apr 3, 2024 · d71260a · d71260a
1 parent 0576e8f
commit d71260a
Show file tree

Hide file tree

Showing 12 changed files with 197 additions and 41 deletions.
diff --git a/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/AstDumper.hpp b/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/AstDumper.hpp
@@ -61,8 +61,8 @@ class AstDumper {
   virtual void AddDocumentRangeEnd() = 0;
   virtual void AddDeprecatedRangeStart() = 0;
   virtual void AddDeprecatedRangeEnd() = 0;
-  virtual void AddDiffRangeStart() = 0;
-  virtual void AddDiffRangeEnd() = 0;
+  virtual void AddSkipDiffRangeStart() = 0;
+  virtual void AddSkipDiffRangeEnd() = 0;
 
   virtual void DumpTypeHierarchyNode(std::shared_ptr<TypeHierarchy::TypeHierarchyNode> const& node)
       = 0;

diff --git a/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/AstNode.cpp b/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/AstNode.cpp
@@ -57,7 +57,7 @@ struct AstTerminalNode : public AstNode
 };
 
 /** An AST Type represents a type in the C++ language.
- * 
+ *
  */
 class AstType {
 
@@ -984,6 +984,12 @@ class AstAttribute : public AstNode {
         m_deprecatedReplacement = deprecated->getReplacement();
         break;
       }
+      case attr::Kind::WarnUnusedResult: {
+        auto nodiscard = cast<WarnUnusedResultAttr>(attribute);
+        m_nodiscardMessage = nodiscard->getMessage();
+        break;
+      }
+
       case attr::Kind::CXX11NoReturn:
         break;
       default: {
@@ -1009,6 +1015,9 @@ class AstAttribute : public AstNode {
       case attr::Kind::Deprecated:
         DumpDeprecated(dumper, options);
         break;
+      case attr::Kind::WarnUnusedResult:
+        DumpNoDiscard(dumper, options);
+        break;
       case attr::Kind::CXX11NoReturn:
         dumper->InsertPunctuation('[');
         dumper->InsertPunctuation('[');
@@ -1024,6 +1033,37 @@ class AstAttribute : public AstNode {
     }
   }
 
+  void DumpNoDiscard(AstDumper* dumper, DumpNodeOptions const& options) const
+  {
+    if (options.NeedsLeftAlign)
+    {
+      dumper->LeftAlign();
+    }
+    switch (m_syntax)
+    {
+      case Attr::Syntax::AS_C2x:
+      case Attr::Syntax::AS_CXX11:
+        dumper->InsertPunctuation('[');
+        dumper->InsertPunctuation('[');
+        dumper->InsertKeyword(m_attributeName);
+        if (!m_nodiscardMessage.empty())
+        {
+          dumper->InsertPunctuation('(');
+          dumper->InsertPunctuation('"');
+          dumper->InsertLiteral(m_nodiscardMessage);
+          dumper->InsertPunctuation('"');
+          dumper->InsertPunctuation(')');
+        }
+        dumper->InsertPunctuation(']');
+        dumper->InsertPunctuation(']');
+        break;
+
+      default:
+        llvm::outs() << "UNknown syntax for nodiscard\n";
+        break;
+    }
+  }
+
   void DumpDeprecated(AstDumper* dumper, DumpNodeOptions const& options) const
   {
     if (options.NeedsLeftAlign)
@@ -1109,6 +1149,7 @@ class AstAttribute : public AstNode {
   std::string m_attributeName;
   std::string m_deprecatedMessage;
   std::string m_deprecatedReplacement;
+  std::string m_nodiscardMessage;
 };
 
 AstNamedNode::AstNamedNode(
@@ -1196,8 +1237,8 @@ void AstNamedNode::DumpDocumentation(AstDumper* dumper, DumpNodeOptions const& o
         innerOptions.NeedsTrailingNewline = false;
         m_nodeDocumentation->DumpNode(dumper, innerOptions);
       }
-      dumper->Newline(); // We need to insert a newline here to ensure that the comment is properly
-                         // closed.
+      dumper->Newline(); // We need to insert a newline here to ensure that the comment is
+                         // properly closed.
       dumper->LeftAlign();
       dumper->InsertComment(" */");
       if (options.NeedsTrailingNewline)
@@ -1215,6 +1256,7 @@ void AstNamedNode::DumpSourceComment(AstDumper* dumper, DumpNodeOptions const& o
 {
   if (options.NeedsSourceComment)
   {
+    dumper->AddSkipDiffRangeStart();
     if (options.NeedsLeadingNewline)
     {
       dumper->Newline();
@@ -1238,6 +1280,7 @@ void AstNamedNode::DumpSourceComment(AstDumper* dumper, DumpNodeOptions const& o
     {
       dumper->Newline();
     }
+    dumper->AddSkipDiffRangeEnd();
   }
 }
 
@@ -1960,14 +2003,14 @@ class AstMethod : public AstFunction {
       : AstFunction(method, database, parentNode), m_isVirtual(method->isVirtual()),
         m_isPure(method->isPure()), m_isConst(method->isConst())
   {
-    // We assume that this is an implicit override if there are overriden methods. If we later find
-    // an override attribute, we know it's not an implicit override.
+    // We assume that this is an implicit override if there are overriden methods. If we later
+    // find an override attribute, we know it's not an implicit override.
     //
     // Note that we don't do this for destructors, because they typically won't have an override
     // attribute.
     //
-    // Also note that if size_overriden_methods is non-zero, it means that the base class method is
-    // already virtual.
+    // Also note that if size_overriden_methods is non-zero, it means that the base class method
+    // is already virtual.
     //
     if (method->getKind() == Decl::Kind::CXXMethod)
     {
@@ -2001,7 +2044,9 @@ class AstMethod : public AstFunction {
           }
           break;
         case attr::Deprecated:
+        case attr::WarnUnusedResult:
           break;
+
         default:
           llvm::outs() << "Unknown Method Attribute: ";
           attr->printPretty(llvm::outs(), LangOptions());
@@ -2648,25 +2693,59 @@ class AstFriend : public AstNode {
   std::unique_ptr<AstNode> m_friendFunction;
 
 public:
+  static bool ShouldIncludeFriendDeclaration(FriendDecl const* friendDecl)
+  {
+    if (!friendDecl->getFriendType() && !friendDecl->getFriendDecl())
+    {
+      return false;
+    }
+
+    if (friendDecl->getFriendType())
+    {
+      auto friendTypeName
+          = QualType::getAsString(friendDecl->getFriendType()->getType().split(), LangOptions{});
+      // If the friend type is in the std namespace, we don't want to include it.
+      if (friendTypeName.find("std::") == 0)
+      {
+        return false;
+      }
+
+      // If the friend type is in the detail namespace, we don't want to include it.
+      if (friendTypeName.find("_detail::") != std::string::npos)
+      {
+        return false;
+      }
+    }
+
+    return true;
+  }
   AstFriend(
       FriendDecl const* friendDecl,
       AzureClassesDatabase* const azureClassesDatabase,
       std::shared_ptr<TypeHierarchy::TypeHierarchyNode> parentNode)
       : AstNode()
   {
-    if (friendDecl->getFriendType())
+    if (ShouldIncludeFriendDeclaration(friendDecl))
     {
-      m_friendType
-          = QualType::getAsString(friendDecl->getFriendType()->getType().split(), LangOptions{});
-    }
-    else if (friendDecl->getFriendDecl())
-    {
-      m_friendFunction
-          = AstNode::Create(friendDecl->getFriendDecl(), azureClassesDatabase, parentNode);
+      if (friendDecl->getFriendType())
+      {
+        m_friendType
+            = QualType::getAsString(friendDecl->getFriendType()->getType().split(), LangOptions{});
+      }
+      else if (friendDecl->getFriendDecl())
+      {
+        m_friendFunction
+            = AstNode::Create(friendDecl->getFriendDecl(), azureClassesDatabase, parentNode);
+      }
     }
   }
   void DumpNode(AstDumper* dumper, DumpNodeOptions const& dumpOptions) const override
   {
+    // If we're skipping this friend declaration, don't do anything.
+    if (m_friendFunction == nullptr && m_friendType.empty())
+    {
+      return;
+    }
     if (dumpOptions.NeedsLeftAlign)
     {
       dumper->LeftAlign();
@@ -2684,7 +2763,7 @@ class AstFriend : public AstNode {
     }
     else
     {
-      dumper->InsertIdentifier(m_friendType);
+      dumper->InsertTypeName(m_friendType, "friend_" + m_friendType);
       if (dumpOptions.NeedsTrailingSemi)
       {
         dumper->InsertPunctuation(';');
@@ -2949,8 +3028,8 @@ AstClassLike::AstClassLike(
         if (child->getAccess() == AS_private) //&& !options.IncludePrivate)
         {
           shouldIncludeChild = false;
-          // If the method is a virtual method, then we need to include it because it's functionally
-          // a protected method.
+          // If the method is a virtual method, then we need to include it because it's
+          // functionally a protected method.
           if (child->getKind() == Decl::Kind::CXXMethod)
           {
             if (cast<CXXMethodDecl>(child)->isVirtual())

diff --git a/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/AstNode.hpp b/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/AstNode.hpp
@@ -40,7 +40,9 @@ class AstNode {
 protected:
   explicit AstNode();
 
+
 public:
+  virtual ~AstNode() = default;
   // AstNode's don't have namespaces or names, so return something that would make callers happy.
   virtual std::string_view const Namespace() const { return ""; }
   virtual std::string_view const Name() const { return ""; }

diff --git a/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/CMakeLists.txt b/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/CMakeLists.txt
@@ -48,6 +48,7 @@ clangSerialization
 clangTooling
 clangEdit
 clangAnalysis
+clangASTMatchers
 )
 
 

diff --git a/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/JsonDumper.hpp b/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/JsonDumper.hpp
@@ -269,15 +269,15 @@ class JsonDumper : public AstDumper {
          {"Value", nullptr},
          {"Kind", TokenKinds::DeprecatedRangeEnd}});
   }
-  virtual void AddDiffRangeStart() override
+  virtual void AddSkipDiffRangeStart() override
   {
     m_json["Tokens"].push_back(
         {{"DefinitionId", nullptr},
          {"NavigateToId", nullptr},
          {"Value", nullptr},
          {"Kind", TokenKinds::SkipDiffRangeStart}});
   }
-  virtual void AddDiffRangeEnd() override
+  virtual void AddSkipDiffRangeEnd() override
   {
     m_json["Tokens"].push_back(
         {{"DefinitionId", nullptr},

diff --git a/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/TextDumper.hpp b/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/TextDumper.hpp
@@ -81,8 +81,8 @@ class TextDumper : public AstDumper {
   virtual void AddExternalLinkEnd() override { m_stream << "**LINK**</a>**LINK**"; }
   virtual void AddDeprecatedRangeStart() override { m_stream << "/* ** DEPRECATED **"; }
   virtual void AddDeprecatedRangeEnd() override { m_stream << "/* ** DEPRECATED ** */"; }
-  virtual void AddDiffRangeStart() override { m_stream << "/* ** DIFF **"; }
-  virtual void AddDiffRangeEnd() override { m_stream << " ** DIFF ** */"; }
+  virtual void AddSkipDiffRangeStart() override { m_stream << "/* ** SKIP DIFF **"; }
+  virtual void AddSkipDiffRangeEnd() override { m_stream << " ** SKIP DIFF ** */"; }
 
   void DoDumpHierarchyNode(
       std::shared_ptr<TypeHierarchy::TypeHierarchyNode> const& node,

diff --git a/tools/apiview/parsers/cpp-api-parser/ParseTests/CMakeLists.txt b/tools/apiview/parsers/cpp-api-parser/ParseTests/CMakeLists.txt
@@ -19,7 +19,8 @@ add_executable(parseTests
     TestCases/ExpressionTests.cpp
     TestCases/UsingNamespace.cpp
     TestCases/DestructorTests.cpp
-    TestCases/DocumentationTests.cpp)
+    TestCases/FriendsTests.cpp
+    TestCases/DocumentationTests.cpp "TestCases/FriendsTests.cpp")
 add_dependencies(parseTests ApiViewProcessor)
 
 target_include_directories(parseTests PRIVATE ${ApiViewProcessor_SOURCE_DIR})

diff --git a/tools/apiview/parsers/cpp-api-parser/ParseTests/TestCases/FriendsTests.cpp b/tools/apiview/parsers/cpp-api-parser/ParseTests/TestCases/FriendsTests.cpp
@@ -0,0 +1,37 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+#include <iostream>
+
+namespace MyTest {
+
+class M1 {
+  int intVal{};
+
+  bool isIntValEqual(const M1& other) const { return intVal == other.intVal; }
+};
+
+namespace _detail {
+
+  class DetailedClass {
+    int intVal{};
+
+    bool isIntValEqual(const DetailedClass& other) const { return intVal == other.intVal; }
+  };
+} // namespace _detail
+
+class M2 {
+  int intVal{};
+
+  bool isIntValEqual(const M2& other) const { return intVal == other.intVal; }
+  friend class M1;
+  friend class _detail::DetailedClass;
+  friend std::ostream& operator<<(std::ostream& os, const M2& m2);
+};
+
+std::ostream& operator<<(std::ostream& os, const M2& m2)
+{
+  os << m2.intVal;
+  return os;
+}
+} // namespace MyTest