Issue chipsalliance#3221, chipsalliance#3219: Follow up and fixing wa…

…rnings

* 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

@@ -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

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)

include/Surelog/Common/NodeId.h

@@ -117,4 +117,4 @@ typedef std::unordered_set<NodeId, NodeIdHasher, NodeIdEqualityComparer>
 
 }  // namespace SURELOG
 
-#endif  // SURELOG_NODEID_H
+#endif  // SURELOG_NODEID_H

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;

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,15 +36,15 @@ 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) {}
   SymbolId(RawSymbolId id, std::string_view value) : id(id) {}
   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) {

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();

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 <Surelog/Common/FileSystem.h>
+#include <Surelog/Common/PathId.h>
+#include <Surelog/SourceCompile/SymbolTable.h>
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include <filesystem>
+#include <memory>
+
+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<SymbolTable> symbolTableA(new SymbolTable);
+  std::unique_ptr<SymbolTable> symbolTableB(new SymbolTable);
+  std::unique_ptr<FileSystem> 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<SymbolTable> symbolTableA(new SymbolTable);
+  std::unique_ptr<FileSystem> 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<SymbolTable> symbolTableA(new SymbolTable);
+  std::unique_ptr<FileSystem> 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<SymbolTable> 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

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

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 =