Closed paragbansal closed 9 months ago
@paragbansal thanks for the report, but I am not able to reproduce it on my MacBook. I added -Woverloaded-virtual
in CMakeLists.txt as below, and run mkdir build & cd build && cmake .. && make
.
Thank you @yhirose for having a quick look. I can reproduce using a gcc compiler on Mac.
brew install gcc
% git diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index fa15835..59d859c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1,13 +1,15 @@
cmake_minimum_required(VERSION 3.14)
-project(peglib)
+set(CMAKE_C_COMPILER /opt/homebrew/Cellar/gcc/13.2.0/bin/gcc-13)
+set(CMAKE_CXX_COMPILER /opt/homebrew/Cellar/gcc/13.2.0/bin/g++-13)
+project(peglib)
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_EXTENSIONS OFF)
if(MSVC)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Zc:__cplusplus /utf-8 /D_CRT_SECURE_NO_DEPRECATE")
else()
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Woverloaded-virtual")
endif()
set(THREADS_PREFER_PTHREAD_FLAG ON)
% make
[ 8%] Building CXX object _deps/googletest-build/googletest/CMakeFiles/gtest.dir/src/gtest-all.cc.o
[ 16%] Linking CXX static library ../../../lib/libgtest.a
[ 16%] Built target gtest
[ 25%] Building CXX object _deps/googletest-build/googletest/CMakeFiles/gtest_main.dir/src/gtest_main.cc.o
[ 33%] Linking CXX static library ../../../lib/libgtest_main.a
[ 33%] Built target gtest_main
[ 41%] Building CXX object test/CMakeFiles/peglib-test-main.dir/test1.cc.o
In file included from /Users/paragbansal/cpp-peglib/test/test1.cc:2:
/Users/paragbansal/cpp-peglib/test/../peglib.h:1687:16: warning: 'virtual void peg::Ope::Visitor::visit(peg::Cut&)' was hidden [-Woverloaded-virtual=]
1687 | virtual void visit(Cut &) {}
| ^~~~~
/Users/paragbansal/cpp-peglib/test/../peglib.h:1748:8: note: by 'peg::AssignIDToDefinition::visit'
1748 | void visit(Recovery &ope) override { ope.ope_->accept(*this); }
@paragbansal thanks for your help!
Compiling with -Woverloaded-virtual fails.
This was reported in #118 as well, but seems like the change got removed. Just wanted to see if any particular reason to?