NJU-ProjectN / nvboard

NJU Virtual Board
Other
198 stars 38 forks source link

Clean up headers inclusion in headers #25

Closed xinyangli closed 6 months ago

xinyangli commented 6 months ago

Currently, nvboard.h includes almost all of the other headers, and C++ source files in the project include nvboard.h to access definitions and macros. However, external library users also use this header file, which exposes many symbols that are not needed, such as str() and str_temp(). This can cause problems since the name str() is very common in user code and libraries.

To provide a minimum reproducible example, apply the following patch to the example:

diff --git a/example/Makefile b/example/Makefile
index 845a29b..adfdedc 100644
--- a/example/Makefile
+++ b/example/Makefile
@@ -33,7 +33,7 @@ CXXFLAGS += $(INCFLAGS) -DTOP_NAME="\"V$(TOPNAME)\""

 $(BIN): $(VSRCS) $(CSRCS) $(NVBOARD_ARCHIVE)
        @rm -rf $(OBJ_DIR)
-       $(VERILATOR) $(VERILATOR_CFLAGS) \
+       $(VERILATOR) $(VERILATOR_CFLAGS) --coverage \
                --top-module $(TOPNAME) $^ \
                $(addprefix -CFLAGS , $(CXXFLAGS)) $(addprefix -LDFLAGS , $(LDFLAGS)) \
                --Mdir $(OBJ_DIR) --exe -o $(abspath $(BIN))

This will not compile because our macro str() will cause a compile error in the following line in verilator_cov.h:

    return os.str();

PS: This issue can be circumvented by including Vtop.h first in auto_bind.cpp.

We can address this by limiting the functions and macros exposed in nvboard.h. Additionally, it's generally better practice to include only the necessary headers in header files, as disscussed here.

I've tested this patch, but we probably need more tests as it might introduce bugs due to missing references.

sashimi-yzh commented 6 months ago

Thank you very much! This have be fixed this in v0.4, which is still under development. I will rebase the v0.4 branch to your patch.