From 7aa2d22bb3207a95cf35abd2eb3f4f20b2385f32 Mon Sep 17 00:00:00 2001 From: HS Date: Fri, 9 Sep 2022 12:23:44 -0700 Subject: [PATCH] Issue #3221, #3219: Follow up and fixing warnings * Fixed build warnings and also updated the workflow to match local settings so the warnings should now be reported on CI as well. * Add unit tests for PathId * Some documentation & decorative changes --- .github/workflows/main.yml | 4 +- CMakeLists.txt | 13 ++-- include/Surelog/Common/NodeId.h | 2 +- include/Surelog/Common/PathId.h | 29 +++++++++ include/Surelog/Common/SymbolId.h | 6 +- src/Cache/PPCache.cpp | 1 - src/Common/PathId_test.cpp | 97 +++++++++++++++++++++++++++++ src/DesignCompile/CompileHelper.cpp | 1 - src/DesignCompile/UhdmWriter.cpp | 1 - src/SourceCompile/Compiler.cpp | 2 - 10 files changed, 139 insertions(+), 17 deletions(-) create mode 100644 src/Common/PathId_test.cpp diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index bf72346eff..de6954703e 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -73,7 +73,7 @@ jobs: run: | echo 'PATH=/usr/lib/ccache:'"$PATH" >> $GITHUB_ENV echo 'PREFIX=${GITHUB_WORKSPACE}/install' >> $GITHUB_ENV - echo "ADDITIONAL_CMAKE_OPTIONS=-DPython3_ROOT_DIR=${pythonLocation} -DMY_CXX_WARNING_FLAGS='-W -Wall -Wextra -Wno-unused-parameter -Wno-unused-variable -Werror -UNDEBUG'" >> $GITHUB_ENV + echo "ADDITIONAL_CMAKE_OPTIONS=-DPython3_ROOT_DIR=${pythonLocation} -DMY_CXX_WARNING_FLAGS='-W -Wall -Wextra -Wno-unused-parameter -Werror -UNDEBUG'" >> $GITHUB_ENV echo "CMAKE_GENERATOR=Ninja" >> $GITHUB_ENV - name: Show shell configuration @@ -180,7 +180,7 @@ jobs: echo 'CXX=g++-9' >> $GITHUB_ENV echo 'PATH=/usr/lib/ccache:'"$PATH" >> $GITHUB_ENV echo 'PREFIX=${GITHUB_WORKSPACE}/install' >> $GITHUB_ENV - echo "ADDITIONAL_CMAKE_OPTIONS=-DPython3_ROOT_DIR=${pythonLocation} -DMY_CXX_WARNING_FLAGS='-W -Wall -Wextra -Wno-unused-parameter -Wno-unused-variable -Werror -UNDEBUG'" >> $GITHUB_ENV + echo "ADDITIONAL_CMAKE_OPTIONS=-DPython3_ROOT_DIR=${pythonLocation} -DMY_CXX_WARNING_FLAGS='-W -Wall -Wextra -Wno-unused-parameter -Werror -UNDEBUG'" >> $GITHUB_ENV echo "CMAKE_GENERATOR=Ninja" >> $GITHUB_ENV - name: Show shell configuration diff --git a/CMakeLists.txt b/CMakeLists.txt index df2fa116cb..a6911e979a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -537,16 +537,17 @@ function(register_gtests) endfunction() register_gtests( - src/Utils/StringUtils_test.cpp - src/Utils/FileUtils_test.cpp - src/SourceCompile/SymbolTable_test.cpp - src/Expression/ExprBuilder_test.cpp - src/SourceCompile/PreprocessFile_test.cpp - src/SourceCompile/ParseFile_test.cpp + src/Common/PathId_test.cpp src/DesignCompile/CompileExpression_test.cpp src/DesignCompile/CompileHelper_test.cpp src/DesignCompile/Elaboration_test.cpp src/DesignCompile/Uhdm_test.cpp + src/Expression/ExprBuilder_test.cpp + src/SourceCompile/ParseFile_test.cpp + src/SourceCompile/PreprocessFile_test.cpp + src/SourceCompile/SymbolTable_test.cpp + src/Utils/FileUtils_test.cpp + src/Utils/StringUtils_test.cpp ) if (NOT QUICK_COMP) diff --git a/include/Surelog/Common/NodeId.h b/include/Surelog/Common/NodeId.h index 02e91b9cd6..81d8851f6a 100644 --- a/include/Surelog/Common/NodeId.h +++ b/include/Surelog/Common/NodeId.h @@ -117,4 +117,4 @@ typedef std::unordered_set } // namespace SURELOG -#endif // SURELOG_NODEID_H \ No newline at end of file +#endif // SURELOG_NODEID_H diff --git a/include/Surelog/Common/PathId.h b/include/Surelog/Common/PathId.h index 9800c4b992..5b34cad405 100644 --- a/include/Surelog/Common/PathId.h +++ b/include/Surelog/Common/PathId.h @@ -1,3 +1,19 @@ +/* + Copyright 2019 Alain Dargelas + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + #ifndef SURELOG_PATHID_H #define SURELOG_PATHID_H #pragma once @@ -22,6 +38,19 @@ #endif namespace SURELOG { +/** + * class PathId + * + * Used to uniquely represent a file or directory abstractly. + * The context/value doesn't have to be a std::filesystem::path but + * can be any printable (i.e. convertible to string) value. This pinned + * value is stored in the PathId::m_symbolTable. + * + * All operations on/with PathId has to go through the SURELOG::FileSystem. + * Logic in Surelog (or client application) shouldn't access/alter the value + * of it outside of the SURELOG::FileSystem context. + * + */ typedef uint32_t RawPathId; inline static constexpr RawPathId BadRawPathId = 0; inline static constexpr std::string_view BadRawPath = BadRawSymbol; diff --git a/include/Surelog/Common/SymbolId.h b/include/Surelog/Common/SymbolId.h index 626771c40c..6010630564 100644 --- a/include/Surelog/Common/SymbolId.h +++ b/include/Surelog/Common/SymbolId.h @@ -23,7 +23,7 @@ namespace SURELOG { * * Used to uniquely represent a string in SymbolTable. SymbolId can (and * should) be resolved only with the SymbolTable that it was generated with. - * + * */ typedef uint32_t RawSymbolId; inline static constexpr RawSymbolId BadRawSymbolId = 0; @@ -36,7 +36,7 @@ class SymbolId final { public: #if SYMBOLID_DEBUG_ENABLED SymbolId() : id(BadRawSymbolId), value(BadRawSymbol) {} - SymbolId(RawSymbolId id, std::string_view value) : id(id) , value(value) {} + SymbolId(RawSymbolId id, std::string_view value) : id(id), value(value) {} SymbolId(const SymbolId &rhs) : id(rhs.id), value(rhs.value) {} #else SymbolId() : id(BadRawSymbolId) {} @@ -44,7 +44,7 @@ class SymbolId final { SymbolId(const SymbolId &rhs) : id(rhs.id) {} #endif - explicit SymbolId(const PathId &rhs); // Implementation in Path.h + explicit SymbolId(const PathId &rhs); // Implementation in Path.h SymbolId &operator=(const SymbolId &rhs) { if (this != &rhs) { diff --git a/src/Cache/PPCache.cpp b/src/Cache/PPCache.cpp index 2871494e5e..b5d6cff138 100644 --- a/src/Cache/PPCache.cpp +++ b/src/Cache/PPCache.cpp @@ -134,7 +134,6 @@ bool PPCache::restore_(PathId cacheFileId, /* Restore `timescale directives */ if (!errorsOnly) { - SymbolTable* localSymbols = m_pp->getCompileSourceFile()->getSymbolTable(); for (const CACHE::TimeInfo* fbtimeinfo : *ppcache->time_info()) { TimeInfo timeInfo; timeInfo.m_type = (TimeInfo::Type)fbtimeinfo->type(); diff --git a/src/Common/PathId_test.cpp b/src/Common/PathId_test.cpp new file mode 100644 index 0000000000..6cf31653f0 --- /dev/null +++ b/src/Common/PathId_test.cpp @@ -0,0 +1,97 @@ +/* + Copyright 2021 Alain Dargelas + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +#include +#include +#include +#include +#include + +#include +#include + +namespace SURELOG { + +using ::testing::ElementsAre; +namespace fs = std::filesystem; + +namespace { +class TestFileSystem : public FileSystem { + public: + TestFileSystem() : FileSystem(std::filesystem::current_path()) {} +}; + +TEST(PathId, constructors) { + std::unique_ptr symbolTableA(new SymbolTable); + std::unique_ptr symbolTableB(new SymbolTable); + std::unique_ptr fileSystem(new TestFileSystem); + + PathId pathId1a = fileSystem->toPathId(fs::path(testing::TempDir()) / "path1", + symbolTableA.get()); + PathId pathId2a = fileSystem->toPathId(fs::path(testing::TempDir()) / "path2", + symbolTableA.get()); + + PathId pathId1b = fileSystem->toPathId(fs::path(testing::TempDir()) / "path1", + symbolTableB.get()); + PathId pathId2b = fileSystem->toPathId(fs::path(testing::TempDir()) / "path2", + symbolTableB.get()); + + EXPECT_EQ(pathId1a.getSymbolTable(), pathId2a.getSymbolTable()); + EXPECT_EQ(pathId1b.getSymbolTable(), pathId2b.getSymbolTable()); + EXPECT_NE(pathId1a.getSymbolTable(), pathId1b.getSymbolTable()); + + PathId pathId3a(pathId1a); + EXPECT_EQ(pathId1a, pathId3a); + EXPECT_EQ(pathId1a.getSymbolTable(), pathId3a.getSymbolTable()); +} + +TEST(PathId, assignment_operator) { + std::unique_ptr symbolTableA(new SymbolTable); + std::unique_ptr fileSystem(new TestFileSystem); + + PathId pathId1 = fileSystem->toPathId(fs::path(testing::TempDir()) / "path1", + symbolTableA.get()); + + PathId pathId2 = pathId1; + EXPECT_EQ(pathId1, pathId2); + EXPECT_EQ(pathId1.getSymbolTable(), pathId2.getSymbolTable()); +} + +TEST(PathId, comparison_operators) { + std::unique_ptr symbolTableA(new SymbolTable); + std::unique_ptr fileSystem(new TestFileSystem); + + PathId pathId1 = fileSystem->toPathId(fs::path(testing::TempDir()) / "path1", + symbolTableA.get()); + PathId pathId2 = fileSystem->toPathId(fs::path(testing::TempDir()) / "path2", + symbolTableA.get()); + + PathId pathId4, pathId5; + EXPECT_EQ(pathId4, pathId4); // Both empty self + EXPECT_EQ(pathId1, pathId1); // Against non-empty self + EXPECT_EQ(pathId4, pathId5); // Two distinct empty + EXPECT_NE(pathId1, pathId4); // Empty against not + EXPECT_NE(pathId1, pathId2); // Two distinct non-empty + + std::unique_ptr symbolTableB(new SymbolTable); + PathId pathId6 = fileSystem->toPathId(fs::path(testing::TempDir()) / "path1", + symbolTableB.get()); + EXPECT_NE(pathId1.getSymbolTable(), pathId6.getSymbolTable()); + EXPECT_EQ(pathId1, pathId6); // Same stored in distinct SymbolTables +} + +} // namespace +} // namespace SURELOG diff --git a/src/DesignCompile/CompileHelper.cpp b/src/DesignCompile/CompileHelper.cpp index cd933f564e..eafaae668b 100644 --- a/src/DesignCompile/CompileHelper.cpp +++ b/src/DesignCompile/CompileHelper.cpp @@ -78,7 +78,6 @@ void CompileHelper::checkForLoops(bool on) { bool CompileHelper::loopDetected(PathId fileId, int lineNumber, CompileDesign* compileDesign, ValuedComponentI* instance) { - FileSystem* const fileSystem = FileSystem::getInstance(); #if defined(_WIN32) constexpr int32_t kMaxAllowedStackDepth = 100; #else diff --git a/src/DesignCompile/UhdmWriter.cpp b/src/DesignCompile/UhdmWriter.cpp index 933e6435bd..0134f0d0c0 100644 --- a/src/DesignCompile/UhdmWriter.cpp +++ b/src/DesignCompile/UhdmWriter.cpp @@ -3608,7 +3608,6 @@ vpiHandle UhdmWriter::write(PathId uhdmFileId) { for (const IncludeFileInfo& ifi : pf->getIncludeFileInfo()) { if ((ifi.m_context == IncludeFileInfo::Context::INCLUDE) && (ifi.m_action == IncludeFileInfo::Action::PUSH)) { - const FileContent* const fC = pf->getFileContent(); include_file_info* const pifi = s.MakeInclude_file_info(); pifi->VpiFile(fileSystem->toPath(pf->getRawFileId()).string()); pifi->VpiIncludedFile(fileSystem->toPath(ifi.m_sectionFile).string()); diff --git a/src/SourceCompile/Compiler.cpp b/src/SourceCompile/Compiler.cpp index 96b34d4bef..d9d02b8668 100644 --- a/src/SourceCompile/Compiler.cpp +++ b/src/SourceCompile/Compiler.cpp @@ -368,7 +368,6 @@ bool Compiler::createMultiProcessParser_() { // Create CMakeLists.txt bool muted = m_commandLineParser->muteStdout(); - SymbolTable* symbolTable = getSymbolTable(); const fs::path outputDir = fileSystem->toPath(m_commandLineParser->getOutputDirId()); const fs::path directory = @@ -551,7 +550,6 @@ bool Compiler::createMultiProcessPreProcessor_() { // Create CMakeLists.txt bool muted = m_commandLineParser->muteStdout(); - SymbolTable* symbolTable = getSymbolTable(); const fs::path outputDir = fileSystem->toPath(m_commandLineParser->getOutputDirId()); const fs::path directory =