From d71260ae6337c1da7cb6ffcdd3e2e45df3b63fd8 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Wed, 3 Apr 2024 10:59:31 -0700 Subject: [PATCH] C++ ApiView: Add Support for the [[nodiscard]] attribute; clean up some 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 --- .../ApiViewProcessor/AstDumper.hpp | 4 +- .../ApiViewProcessor/AstNode.cpp | 115 +++++++++++++++--- .../ApiViewProcessor/AstNode.hpp | 2 + .../ApiViewProcessor/CMakeLists.txt | 1 + .../ApiViewProcessor/JsonDumper.hpp | 4 +- .../ApiViewProcessor/TextDumper.hpp | 4 +- .../cpp-api-parser/ParseTests/CMakeLists.txt | 3 +- .../ParseTests/TestCases/FriendsTests.cpp | 37 ++++++ .../cpp-api-parser/ParseTests/tests.cpp | 52 ++++++-- tools/apiview/parsers/cpp-api-parser/ci.yml | 4 +- .../cmake-modules/AzureVcpkg.cmake | 2 +- .../apiview/parsers/cpp-api-parser/vcpkg.json | 10 +- 12 files changed, 197 insertions(+), 41 deletions(-) create mode 100644 tools/apiview/parsers/cpp-api-parser/ParseTests/TestCases/FriendsTests.cpp diff --git a/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/AstDumper.hpp b/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/AstDumper.hpp index 4b68b06feac..ff71cbe0679 100644 --- 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 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 index dc043cad33b..7f3c90761dc 100644 --- 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(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 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 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(child)->isVirtual()) diff --git a/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/AstNode.hpp b/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/AstNode.hpp index 4e1b195b319..95b8b2cb183 100644 --- 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 index 5695b0e87d0..c1a1dbe423b 100644 --- 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 index fa65ea9dece..aa4a66da499 100644 --- a/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/JsonDumper.hpp +++ b/tools/apiview/parsers/cpp-api-parser/ApiViewProcessor/JsonDumper.hpp @@ -269,7 +269,7 @@ class JsonDumper : public AstDumper { {"Value", nullptr}, {"Kind", TokenKinds::DeprecatedRangeEnd}}); } - virtual void AddDiffRangeStart() override + virtual void AddSkipDiffRangeStart() override { m_json["Tokens"].push_back( {{"DefinitionId", nullptr}, @@ -277,7 +277,7 @@ class JsonDumper : public AstDumper { {"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 index ae1653053c1..8ef87cc9aba 100644 --- 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****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 const& node, diff --git a/tools/apiview/parsers/cpp-api-parser/ParseTests/CMakeLists.txt b/tools/apiview/parsers/cpp-api-parser/ParseTests/CMakeLists.txt index 05d2a9d071f..4c3b383a496 100644 --- 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 new file mode 100644 index 00000000000..76ed5570dae --- /dev/null +++ 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 + +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 diff --git a/tools/apiview/parsers/cpp-api-parser/ParseTests/tests.cpp b/tools/apiview/parsers/cpp-api-parser/ParseTests/tests.cpp index 39b78815755..2f15ed03fa9 100644 --- a/tools/apiview/parsers/cpp-api-parser/ParseTests/tests.cpp +++ b/tools/apiview/parsers/cpp-api-parser/ParseTests/tests.cpp @@ -99,16 +99,21 @@ class TestParser : public testing::Test { std::vector const& additionalIncludePaths, std::vector const& additionalArguments) : CompilationDatabase(), m_filesToCompile(filesToCompile), - m_sourceLocation(std::filesystem::absolute(sourceLocation)), m_additionalIncludePaths{ - additionalIncludePaths} + m_sourceLocation(std::filesystem::absolute(sourceLocation)), + m_additionalIncludePaths{additionalIncludePaths} { for (auto const& arg : additionalArguments) { m_additionalArguments.push_back(std::string(arg)); } } - std::vector - defaultCommandLine{"clang++.exe", "-DAZ_RTTI", "-fcxx-exceptions", "-c", "-std=c++14", "-D_ALLOW_COMPILER_AND_STL_VERSION_MISMATCH"}; + std::vector defaultCommandLine{ + "clang++.exe", + "-DAZ_RTTI", + "-fcxx-exceptions", + "-c", + "-std=c++14", + "-D_ALLOW_COMPILER_AND_STL_VERSION_MISMATCH"}; // Inherited via CompilationDatabase virtual std::vector getCompileCommands(llvm::StringRef FilePath) const override { @@ -261,8 +266,8 @@ struct NsDumper : AstDumper virtual void AddDocumentRangeEnd() override {} virtual void AddDeprecatedRangeStart() override {} virtual void AddDeprecatedRangeEnd() override {} - virtual void AddDiffRangeStart() override {} - virtual void AddDiffRangeEnd() override {} + virtual void AddSkipDiffRangeStart() override {} + virtual void AddSkipDiffRangeEnd() override {} virtual void AddExternalLinkStart(std::string_view const& url) override {} virtual void AddExternalLinkEnd() override {} virtual void DumpTypeHierarchyNode( @@ -591,17 +596,45 @@ TEST_F(TestParser, TestDtors) db->DumpClassDatabase(&dumper); EXPECT_EQ(2ul, dumper.Messages.size()); - size_t nonVirtualDestructor= 0; + size_t nonVirtualDestructor = 0; for (const auto& msg : dumper.Messages) { if (msg.DiagnosticId == "CPA000B") { - nonVirtualDestructor+= 1; + nonVirtualDestructor += 1; } } EXPECT_EQ(nonVirtualDestructor, 2ul); } +TEST_F(TestParser, TestFriends) +{ + ApiViewProcessor processor("tests", R"({ + "sourceFilesToProcess": [ + "FriendsTests.cpp" + ], + "additionalIncludeDirectories": [], + "additionalCompilerSwitches": ["-Qunused-arguments"], + "allowInternal": true, + "includeDetail": false, + "includePrivate": false, + "filterNamespace": null +} +)"_json); + + EXPECT_EQ(processor.ProcessApiView(), 0); + + auto& db = processor.GetClassesDatabase(); + + EXPECT_EQ(4ul, db->GetAstNodeMap().size()); + EXPECT_EQ("M2", db->GetAstNodeMap().at(1)->Name()); + + NsDumper dumper; + db->DumpClassDatabase(&dumper); + + EXPECT_TRUE(SyntaxCheckClassDb(db, "Friends_Test1.cpp")); +} + TEST_F(TestParser, TestDocuments) { ApiViewProcessor processor("tests", R"({ @@ -617,7 +650,6 @@ TEST_F(TestParser, TestDocuments) } )"_json); - EXPECT_EQ(processor.ProcessApiView(), 0); auto& db = processor.GetClassesDatabase(); @@ -627,8 +659,6 @@ TEST_F(TestParser, TestDocuments) db->DumpClassDatabase(&dumper); } - - #if 0 TEST_F(TestParser, AzureCore1) { diff --git a/tools/apiview/parsers/cpp-api-parser/ci.yml b/tools/apiview/parsers/cpp-api-parser/ci.yml index 41e9a4d2baf..e9bb3fa8e8b 100644 --- a/tools/apiview/parsers/cpp-api-parser/ci.yml +++ b/tools/apiview/parsers/cpp-api-parser/ci.yml @@ -32,7 +32,9 @@ stages: timeoutInMinutes: 300 pool: name: azsdk-pool-mms-win-2022-general - vmImage: windows-2022 + os: windows + demands: + - ImageOverride -equals 1espt-win-2022-cpp-rollback steps: - pwsh: | diff --git a/tools/apiview/parsers/cpp-api-parser/cmake-modules/AzureVcpkg.cmake b/tools/apiview/parsers/cpp-api-parser/cmake-modules/AzureVcpkg.cmake index 480edf947ae..8217247f30b 100644 --- a/tools/apiview/parsers/cpp-api-parser/cmake-modules/AzureVcpkg.cmake +++ b/tools/apiview/parsers/cpp-api-parser/cmake-modules/AzureVcpkg.cmake @@ -17,7 +17,7 @@ macro(az_vcpkg_integrate) message("AZURE_SDK_DISABLE_AUTO_VCPKG is not defined. Fetch a local copy of vcpkg.") # GET VCPKG FROM SOURCE # User can set env var AZURE_SDK_VCPKG_COMMIT to pick the VCPKG commit to fetch - set(VCPKG_COMMIT_STRING dafef74af53669ef1cc9015f55e0ce809ead62aa) # default SDK tested commit + set(VCPKG_COMMIT_STRING 9854d1d92200d81dde189e53b64c9ba6a305dc9f) # default SDK tested commit if(DEFINED ENV{AZURE_SDK_VCPKG_COMMIT}) message("AZURE_SDK_VCPKG_COMMIT is defined. Using that instead of the default.") set(VCPKG_COMMIT_STRING "$ENV{AZURE_SDK_VCPKG_COMMIT}") # default SDK tested commit diff --git a/tools/apiview/parsers/cpp-api-parser/vcpkg.json b/tools/apiview/parsers/cpp-api-parser/vcpkg.json index 77048c673f1..753591e85e7 100644 --- a/tools/apiview/parsers/cpp-api-parser/vcpkg.json +++ b/tools/apiview/parsers/cpp-api-parser/vcpkg.json @@ -8,7 +8,11 @@ { "name": "nlohmann-json" }, - { "name": "gtest" }, - { "name": "tclap" } + { + "name": "gtest" + }, + { + "name": "tclap" + } ] - } +}